From 089f13ea8c2db0065d708586fd58775a334360fc Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 8 Apr 2022 15:38:39 +0200 Subject: [PATCH] Trim transitioned Go settings on non-Go dependencies 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 https://github.com/bazelbuild/bazel/issues/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. --- go/private/rules/BUILD.bazel | 14 +++++++ go/private/rules/binary.bzl | 8 ++++ go/private/rules/library.bzl | 11 ++++++ go/private/rules/test.bzl | 8 ++++ go/private/rules/transition.bzl | 66 +++++++++++++++++++++++++++++++-- 5 files changed, 104 insertions(+), 3 deletions(-) diff --git a/go/private/rules/BUILD.bazel b/go/private/rules/BUILD.bazel index 21063d701d..c474a17650 100644 --- a/go/private/rules/BUILD.bazel +++ b/go/private/rules/BUILD.bazel @@ -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"]), diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index 78e74e26af..be30202772 100644 --- a/go/private/rules/binary.bzl +++ b/go/private/rules/binary.bzl @@ -29,6 +29,7 @@ load( ) load( "//go/private/rules:transition.bzl", + "go_non_go_reset_transition", "go_transition_rule", ) load( @@ -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, @@ -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 @@ -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` @@ -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`. @@ -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"], diff --git a/go/private/rules/library.bzl b/go/private/rules/library.bzl index 325f87ecc8..9cabd4ff5f 100644 --- a/go/private/rules/library.bzl +++ b/go/private/rules/library.bzl @@ -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.""" @@ -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. @@ -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, @@ -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. @@ -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`. @@ -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 diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl index a88ebcc91d..b64ce54db5 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -39,6 +39,7 @@ load( ) load( "//go/private/rules:transition.bzl", + "go_non_go_reset_transition", "go_transition_rule", ) load( @@ -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 @@ -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, @@ -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` @@ -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`. @@ -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, diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl index 370b737bc1..d644c99837 100644 --- a/go/private/rules/transition.bzl +++ b/go/private/rules/transition.bzl @@ -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. @@ -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. @@ -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") @@ -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): @@ -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, @@ -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, @@ -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))