Skip to content

Commit

Permalink
Trim transitioned Go settings on non-Go dependencies
Browse files Browse the repository at this point in the history
By resetting Go-specific settings changed by go_transition to their
previous values when crossing a non-deps dependency edge, e.g. srcs or
data, dependencies through those edges are not affected by the change in
Go settings. This has two advantages:

* Correctness: Previously, if a go_binary with linkmode = "c-archive"
  used another go_binary to generate srcs, that go_binary would also be
  built as a c-archive and thus fail to run during the build. This was
  observed in bazelbuild/bazel#14626.
* Performance: With the new Bazel flag
  --experimental_output_directory_naming_scheme=diff_against_baseline,
  transitions can return to the top-level configuration since the
  Starlark settings that were affected by a transition at some point
  during the build are no longer tracked explicitly in a setting, but
  computed as a configuration diff. Resetting the configuration for non-
  deps dependencies thus prevents redundant rebuilds of non-Go deps
  caused by changes in Go settings, achieving a version of
  "configuration trimming" for Go.
  • Loading branch information
fmeum committed Apr 11, 2022
1 parent 8bba4e1 commit 089f13e
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 3 deletions.
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",
"POTENTIALLY_TRANSITIONED_SETTINGS",
)

exports_files(["library.bzl"])

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

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 @@ -29,6 +29,7 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_non_go_reset_transition",
"go_transition_rule",
)
load(
Expand Down Expand Up @@ -160,6 +161,7 @@ _go_binary_kwargs = {
"attrs": {
"srcs": attr.label_list(
allow_files = go_exts + asm_exts + cgo_exts,
cfg = go_non_go_reset_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 @@ -170,6 +172,7 @@ _go_binary_kwargs = {
),
"data": attr.label_list(
allow_files = True,
cfg = go_non_go_reset_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 @@ -197,6 +200,7 @@ _go_binary_kwargs = {
),
"embedsrcs": attr.label_list(
allow_files = True,
cfg = go_non_go_reset_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 @@ -249,6 +253,7 @@ _go_binary_kwargs = {
""",
),
"cdeps": attr.label_list(
cfg = go_non_go_reset_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 @@ -358,6 +363,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",
"go_non_go_reset_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 = go_non_go_reset_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 = go_non_go_reset_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 = go_non_go_reset_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 = go_non_go_reset_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 @@ -39,6 +39,7 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_non_go_reset_transition",
"go_transition_rule",
)
load(
Expand Down Expand Up @@ -190,6 +191,7 @@ _go_test_kwargs = {
"attrs": {
"data": attr.label_list(
allow_files = True,
cfg = go_non_go_reset_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 @@ -199,6 +201,7 @@ _go_test_kwargs = {
),
"srcs": attr.label_list(
allow_files = go_exts + asm_exts + cgo_exts,
cfg = go_non_go_reset_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 @@ -226,6 +229,7 @@ _go_test_kwargs = {
),
"embedsrcs": attr.label_list(
allow_files = True,
cfg = go_non_go_reset_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 @@ -298,6 +302,7 @@ _go_test_kwargs = {
""",
),
"cdeps": attr.label_list(
cfg = go_non_go_reset_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 @@ -403,6 +408,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
66 changes: 63 additions & 3 deletions go/private/rules/transition.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ load(
"platform_from_crosstool",
)

# A list of rules_go settings that are possibly set by go_transition.
POTENTIALLY_TRANSITIONED_SETTINGS = [
"@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 +64,14 @@ def filter_transition_label(label):
else:
return str(Label(label))

def _original_value_setting(setting):
if not "//go/config:" in setting:
return None
name = setting.split(":")[1]
return filter_transition_label("@io_bazel_rules_go//go/private/rules:original_" + name)

_ORIGINAL_VALUE_SETTINGS = [_original_value_setting(setting) for setting in POTENTIALLY_TRANSITIONED_SETTINGS]

def go_transition_wrapper(kind, transition_kind, name, **kwargs):
"""Wrapper for rules that may use transitions.
Expand Down Expand Up @@ -111,11 +129,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 +195,23 @@ def _go_transition_impl(settings, attr):
linkmode_label = filter_transition_label("@io_bazel_rules_go//go/config:linkmode")
settings[linkmode_label] = linkmode

for setting, value in settings.items():
original_value_setting = _original_value_setting(setting)
if not original_value_setting:
continue

# If the outgoing configuration would differ from the incoming one in a
# value, record the old value in the special original_* setting 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[setting]:
# Encoding as JSON makes it possible to embed settings of arbitrary
# types (currently bool, string and string_list) into a string
# setting.
settings[original_value_setting] = json.encode(original_settings[setting])
else:
settings[original_value_setting] = ""

return settings

def _request_nogo_transition(settings, attr):
Expand Down Expand Up @@ -218,10 +257,10 @@ go_transition = transition(
"@io_bazel_rules_go//go/config:pure",
"@io_bazel_rules_go//go/config:tags",
"@io_bazel_rules_go//go/config:linkmode",
]],
]] + _ORIGINAL_VALUE_SETTINGS,
)

_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 +269,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_VALUE_SETTINGS})

_reset_transition_dict = dict(_common_reset_transition_dict, **{
"@io_bazel_rules_go//go/private:bootstrap_nogo": True,
Expand Down Expand Up @@ -372,6 +411,27 @@ so we can apply both transitions.
""",
)

def _go_non_go_reset_transition_impl(settings, attr):
new_settings = {}
for setting in POTENTIALLY_TRANSITIONED_SETTINGS:
original_setting = _original_value_setting(setting)
original_value = settings[original_setting]
if original_value:
# Reset to the original value and clear it.
new_settings[setting] = json.decode(original_value)
new_settings[original_setting] = ""
else:
new_settings[setting] = settings[setting]
new_settings[original_setting] = settings[original_setting]

return new_settings

go_non_go_reset_transition = transition(
implementation = _go_non_go_reset_transition_impl,
inputs = POTENTIALLY_TRANSITIONED_SETTINGS + _ORIGINAL_VALUE_SETTINGS,
outputs = POTENTIALLY_TRANSITIONED_SETTINGS + _ORIGINAL_VALUE_SETTINGS,
)

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

0 comments on commit 089f13e

Please sign in to comment.