Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trim transitioned Go settings on non-Go dependencies #3108

Merged
merged 5 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions go/private/rules/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,7 +1,21 @@
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("@bazel_skylib//rules:common_settings.bzl", "string_setting")
load(
":transition.bzl",
"TRANSITIONED_GO_SETTING_KEYS",
)

exports_files(["library.bzl"])

[
string_setting(
name = "original_" + setting.split(":")[1],
build_setting_default = "",
visibility = ["//visibility:private"],
)
for setting in TRANSITIONED_GO_SETTING_KEYS
]

filegroup(
name = "all_rules",
srcs = glob(["**/*.bzl"]),
Expand Down
8 changes: 8 additions & 0 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ load(
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"non_go_transition",
)
load(
"//go/private:mode.bzl",
Expand Down Expand Up @@ -173,6 +174,7 @@ _go_binary_kwargs = {
"attrs": {
"srcs": attr.label_list(
allow_files = go_exts + asm_exts + cgo_exts,
cfg = non_go_transition,
doc = """The list of Go source files that are compiled to create the package.
Only `.go` and `.s` files are permitted, unless the `cgo`
attribute is set, in which case,
Expand All @@ -183,6 +185,7 @@ _go_binary_kwargs = {
),
"data": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """List of files needed by this rule at run-time. This may include data files
needed or other programs that may be executed. The [bazel] package may be
used to locate run files; they may appear in different places depending on the
Expand Down Expand Up @@ -210,6 +213,7 @@ _go_binary_kwargs = {
),
"embedsrcs": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """The list of files that may be embedded into the compiled package using
`//go:embed` directives. All files must be in the same logical directory
or a subdirectory as source files. All source files containing `//go:embed`
Expand Down Expand Up @@ -262,6 +266,7 @@ _go_binary_kwargs = {
""",
),
"cdeps": attr.label_list(
cfg = non_go_transition,
doc = """The list of other libraries that the c code depends on.
This can be anything that would be allowed in [cc_library deps]
Only valid if `cgo` = `True`.
Expand Down Expand Up @@ -371,6 +376,9 @@ _go_binary_kwargs = {
""",
),
"_go_context_data": attr.label(default = "//:go_context_data"),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
},
"executable": True,
"toolchains": ["@io_bazel_rules_go//go:toolchain"],
Expand Down
11 changes: 11 additions & 0 deletions go/private/rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ load(
"GoLibrary",
"INFERRED_PATH",
)
load(
"//go/private/rules:transition.bzl",
"non_go_transition",
)

def _go_library_impl(ctx):
"""Implements the go_library() rule."""
Expand Down Expand Up @@ -55,6 +59,7 @@ go_library = rule(
attrs = {
"data": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """
List of files needed by this rule at run-time.
This may include data files needed or other programs that may be executed.
Expand All @@ -64,6 +69,7 @@ go_library = rule(
),
"srcs": attr.label_list(
allow_files = go_exts + asm_exts + cgo_exts,
cfg = non_go_transition,
doc = """
The list of Go source files that are compiled to create the package.
Only `.go` and `.s` files are permitted, unless the `cgo` attribute is set,
Expand Down Expand Up @@ -106,6 +112,7 @@ go_library = rule(
),
"embedsrcs": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """
The list of files that may be embedded into the compiled package using `//go:embed`
directives. All files must be in the same logical directory or a subdirectory as source files.
Expand Down Expand Up @@ -133,6 +140,7 @@ go_library = rule(
""",
),
"cdeps": attr.label_list(
cfg = non_go_transition,
doc = """
List of other libraries that the c code depends on.
This can be anything that would be allowed in [cc_library deps] Only valid if `cgo = True`.
Expand Down Expand Up @@ -164,6 +172,9 @@ go_library = rule(
""",
),
"_go_context_data": attr.label(default = "//:go_context_data"),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
},
toolchains = ["@io_bazel_rules_go//go:toolchain"],
doc = """This builds a Go library from a set of source files that are all part of
Expand Down
8 changes: 8 additions & 0 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ load(
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"non_go_transition",
)
load(
"//go/private:mode.bzl",
Expand Down Expand Up @@ -191,6 +192,7 @@ _go_test_kwargs = {
"attrs": {
"data": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """List of files needed by this rule at run-time. This may include data files
needed or other programs that may be executed. The [bazel] package may be
used to locate run files; they may appear in different places depending on the
Expand All @@ -200,6 +202,7 @@ _go_test_kwargs = {
),
"srcs": attr.label_list(
allow_files = go_exts + asm_exts + cgo_exts,
cfg = non_go_transition,
doc = """The list of Go source files that are compiled to create the package.
Only `.go` and `.s` files are permitted, unless the `cgo`
attribute is set, in which case,
Expand Down Expand Up @@ -227,6 +230,7 @@ _go_test_kwargs = {
),
"embedsrcs": attr.label_list(
allow_files = True,
cfg = non_go_transition,
doc = """The list of files that may be embedded into the compiled package using
`//go:embed` directives. All files must be in the same logical directory
or a subdirectory as source files. All source files containing `//go:embed`
Expand Down Expand Up @@ -299,6 +303,7 @@ _go_test_kwargs = {
""",
),
"cdeps": attr.label_list(
cfg = non_go_transition,
doc = """The list of other libraries that the c code depends on.
This can be anything that would be allowed in [cc_library deps]
Only valid if `cgo` = `True`.
Expand Down Expand Up @@ -404,6 +409,9 @@ _go_test_kwargs = {
default = "//go/tools/builders:lcov_merger",
cfg = "target",
),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
},
"executable": True,
"test": True,
Expand Down
101 changes: 85 additions & 16 deletions go/private/rules/transition.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,32 @@ def filter_transition_label(label):
else:
return str(Label(label))

# A list of rules_go settings that are possibly set by go_transition.
# Keep their package name in sync with the implementation of
# _original_setting_key.
TRANSITIONED_GO_SETTING_KEYS = [
filter_transition_label(key)
for key in [
"//go/config:static",
"//go/config:msan",
"//go/config:race",
"//go/config:pure",
"//go/config:linkmode",
"//go/config:tags",
]
]

def _original_setting_key(key):
if not "//go/config:" in key:
fail("_original_setting_key currently assumes that all Go settings live under //go/config, got: " + key)
name = key.split(":")[1]
return filter_transition_label("//go/private/rules:original_" + name)

_SETTING_KEY_TO_ORIGINAL_SETTING_KEY = {
setting: _original_setting_key(setting)
for setting in TRANSITIONED_GO_SETTING_KEYS
}

def go_transition_wrapper(kind, transition_kind, name, **kwargs):
"""Wrapper for rules that may use transitions.

Expand Down Expand Up @@ -111,11 +137,15 @@ def go_transition_rule(**kwargs):
return rule(**kwargs)

def _go_transition_impl(settings, attr):
# NOTE: Keep the list of rules_go settings set by this transition in sync
# with POTENTIALLY_TRANSITIONED_SETTINGS.
#
# NOTE(bazelbuild/bazel#11409): Calling fail here for invalid combinations
# of flags reports an error but does not stop the build.
# In any case, get_mode should mainly be responsible for reporting
# invalid modes, since it also takes --features flags into account.

original_settings = settings
settings = dict(settings)

_set_ternary(settings, attr, "static")
Expand Down Expand Up @@ -173,6 +203,25 @@ def _go_transition_impl(settings, attr):
linkmode_label = filter_transition_label("@io_bazel_rules_go//go/config:linkmode")
settings[linkmode_label] = linkmode

for key, original_key in _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.items():
old_value = original_settings[key]
value = settings[key]

# If the outgoing configuration would differ from the incoming one in a
# value, record the old value in the special original_* key so that the
# real setting can be reset to this value before the new configuration
# would cross a non-deps dependency edge.
if value != old_value:
# Encoding as JSON makes it possible to embed settings of arbitrary
# types (currently bool, string and string_list) into a single type
# of setting (string) with the information preserved whether the
# original setting wasn't set explicitly (empty string) or was set
# explicitly to its default (always a non-empty string with JSON
# encoding, e.g. "\"\"" or "[]").
settings[original_key] = json.encode(old_value)
else:
settings[original_key] = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set this to None or not setting it at all, to indicate that it's not explicitly set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work: Since the setting have type string, they can only be set to a string, not None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried None? According to the official doc:

settings is a dictionary {String:Object}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, it does indeed return an error with Bazel 4.2.1.


return settings

def _request_nogo_transition(settings, attr):
Expand Down Expand Up @@ -203,25 +252,13 @@ go_transition = transition(
"//command_line_option:cpu",
"//command_line_option:crosstool_top",
"//command_line_option:platforms",
"@io_bazel_rules_go//go/config:static",
"@io_bazel_rules_go//go/config:msan",
"@io_bazel_rules_go//go/config:race",
"@io_bazel_rules_go//go/config:pure",
"@io_bazel_rules_go//go/config:tags",
"@io_bazel_rules_go//go/config:linkmode",
]],
] + TRANSITIONED_GO_SETTING_KEYS],
outputs = [filter_transition_label(label) for label in [
"//command_line_option:platforms",
"@io_bazel_rules_go//go/config:static",
"@io_bazel_rules_go//go/config:msan",
"@io_bazel_rules_go//go/config:race",
"@io_bazel_rules_go//go/config:pure",
"@io_bazel_rules_go//go/config:tags",
"@io_bazel_rules_go//go/config:linkmode",
]],
] + TRANSITIONED_GO_SETTING_KEYS + _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.values()],
)

_common_reset_transition_dict = {
_common_reset_transition_dict = dict({
"@io_bazel_rules_go//go/config:static": False,
"@io_bazel_rules_go//go/config:msan": False,
"@io_bazel_rules_go//go/config:race": False,
Expand All @@ -230,7 +267,7 @@ _common_reset_transition_dict = {
"@io_bazel_rules_go//go/config:debug": False,
"@io_bazel_rules_go//go/config:linkmode": LINKMODE_NORMAL,
"@io_bazel_rules_go//go/config:tags": [],
}
}, **{setting: "" for setting in _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.values()})

_reset_transition_dict = dict(_common_reset_transition_dict, **{
"@io_bazel_rules_go//go/private:bootstrap_nogo": True,
Expand Down Expand Up @@ -366,6 +403,38 @@ go_transition.
""",
)

def _non_go_transition_impl(settings, attr):
"""Sets all Go settings to the values they had before the last go_transition.

non_go_transition sets all of the //go/config settings to the value they had
before the last go_transition. This should be used on all attributes of
go_library/go_binary/go_test that are built in the target configuration and
do not constitute advertise any Go providers.

Examples: This transition is applied to the 'data' attribute of go_binary so
that other Go binaries used at runtime aren't affected by a non-standard
link mode set on the go_binary target, but still use the same top-level
settings such as e.g. race instrumentation.
"""
new_settings = {}
for key, original_key in _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.items():
original_value = settings[original_key]
if original_value:
# Reset to the original value and clear it.
new_settings[key] = json.decode(original_value)
new_settings[original_key] = ""
fmeum marked this conversation as resolved.
Show resolved Hide resolved
else:
new_settings[key] = settings[key]
new_settings[original_key] = settings[original_key]
fmeum marked this conversation as resolved.
Show resolved Hide resolved

return new_settings

non_go_transition = transition(
implementation = _non_go_transition_impl,
inputs = TRANSITIONED_GO_SETTING_KEYS + _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.values(),
outputs = TRANSITIONED_GO_SETTING_KEYS + _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.values(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
outputs = TRANSITIONED_GO_SETTING_KEYS + _SETTING_KEY_TO_ORIGINAL_SETTING_KEY.values(),
outputs = TRANSITIONED_GO_SETTING_KEYS,

can we do this and not setting new_settings[original_key] in the impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By not resetting all settings to their command-line defaults, non-Go dependencies of a transitioned go_binary would still differ from top-level targets in the configuration: They would have the top-level config values in their original_* settings, whereas top-level (untransitioned) targets do not. This would still solve the correctness issue linked in the PR description, but would not address the performance problem.

)

def _check_ternary(name, value):
if value not in ("on", "off", "auto"):
fail('{}: must be "on", "off", or "auto"'.format(name))
Expand Down
Loading