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 3 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
97 changes: 81 additions & 16 deletions go/private/rules/transition.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ load(
"platform_from_crosstool",
)

# 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 = [
"@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:linkmode",
"@io_bazel_rules_go//go/config:tags",
]

def filter_transition_label(label):
"""Transforms transition labels for the current workspace.

Expand All @@ -54,6 +66,14 @@ def filter_transition_label(label):
else:
return str(Label(label))

def _original_setting_key(key):
if not "//go/config:" in key:
return None
name = key.split(":")[1]
return filter_transition_label("@io_bazel_rules_go//go/private/rules:original_" + name)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems filter_transition_label does nothing to a string like this, right? In addition, bazelbuild/bazel#10499 is already fixed. Do we still need filter_transition_label at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

It converts @io_bazel_rules_go//go/config:race to //go/config:race when io_bazel_rules_go is the main repo and returns it unchanged otherwise. It may not be necessary here since the "original_" settings are never reference on the command-line, but using it for some settings and not others seemed even more confusing to me - it would also affect the way transition inputs/outputs are declared below.

The bug is fixed, but the full fix is only available in Bazel 5+. Once the minimum version has been updated to 5.0.0, filter_transition_label can be dropped.


_ORIGINAL_SETTING_KEYS = [_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 +131,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 +197,26 @@ def _go_transition_impl(settings, attr):
linkmode_label = filter_transition_label("@io_bazel_rules_go//go/config:linkmode")
settings[linkmode_label] = linkmode

for key, value in settings.items():
original_key = _original_setting_key(key)
if not original_key:
continue

# 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 != original_settings[key]:
# 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(original_settings[key])
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 +247,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 + _ORIGINAL_SETTING_KEYS],
)

_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 +262,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 _ORIGINAL_SETTING_KEYS})

_reset_transition_dict = dict(_common_reset_transition_dict, **{
"@io_bazel_rules_go//go/private:bootstrap_nogo": True,
Expand Down Expand Up @@ -366,6 +398,39 @@ 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 in TRANSITIONED_GO_SETTING_KEYS:
original_key = _original_setting_key(key)
original_value = 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.

Suggested change
original_key = _original_setting_key(key)
original_value = settings[original_key]
original_key = _original_setting_key(key)
if not original_key or original_key not in settings:
continue

So you can skip original_key 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.

I could implement that change, but could you explain why you think it's needed? If _original_setting_key(key) is None for key in TRANSITIONED_GO_SETTING_KEYS, then that's a bug in this file. I could possibly see adding a check with a fail, but a silent continue would - imo - just make these bugs harder to spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, adding a fail with more debuggable message is better. Because _original_setting_key(key) can return None, it's better to check for that when None is not acceptable.

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 added a commit that pushed the conversion into the construction of a dict that contains the mapping and added a fail in the conversion function. What do you think?

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 + _ORIGINAL_SETTING_KEYS,
outputs = TRANSITIONED_GO_SETTING_KEYS + _ORIGINAL_SETTING_KEYS,
fmeum marked this conversation as resolved.
Show resolved Hide resolved
)

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