From fee2095e0a8219c93206be0b5adecfce6f32eac0 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 10 May 2022 00:48:42 +0200 Subject: [PATCH] Trim transitioned Go settings on non-Go dependencies (#3108) * 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. * Verify transition hermeticity for go_{binary,library,test} The test verifies that Go settings such as gotags and linkmode do not affect dependencies through attributes other than deps. --- 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 | 111 ++++++++++++-- tests/core/transition/hermeticity_test.go | 177 ++++++++++++++++++---- 6 files changed, 281 insertions(+), 48 deletions(-) diff --git a/go/private/rules/BUILD.bazel b/go/private/rules/BUILD.bazel index 3eb8d47be0..2341744bfe 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", + "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"]), diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index 388445c164..8d95020ef9 100644 --- a/go/private/rules/binary.bzl +++ b/go/private/rules/binary.bzl @@ -30,6 +30,7 @@ load( load( "//go/private/rules:transition.bzl", "go_transition_rule", + "non_go_transition", ) load( "//go/private:mode.bzl", @@ -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, @@ -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 @@ -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` @@ -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`. @@ -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", + ), }, "toolchains": ["@io_bazel_rules_go//go:toolchain"], "doc": """This builds an executable from a set of source files, diff --git a/go/private/rules/library.bzl b/go/private/rules/library.bzl index 325f87ecc8..e921d1421b 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", + "non_go_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 = 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. @@ -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, @@ -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. @@ -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`. @@ -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 330ae3590b..7e860d18e4 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -40,6 +40,7 @@ load( load( "//go/private/rules:transition.bzl", "go_transition_rule", + "non_go_transition", ) load( "//go/private:mode.bzl", @@ -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 @@ -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, @@ -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` @@ -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`. @@ -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, diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl index 6a2dfb0c34..6499caecfc 100644 --- a/go/private/rules/transition.bzl +++ b/go/private/rules/transition.bzl @@ -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. @@ -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") @@ -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] = "" + return settings def _request_nogo_transition(settings, attr): @@ -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, @@ -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, @@ -366,6 +403,48 @@ 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 of the setting before go_transition. + new_settings[key] = json.decode(original_value) + else: + new_settings[key] = settings[key] + + # Reset the value of the helper setting to its default for two reasons: + # 1. Performance: This ensures that the Go settings of non-Go + # dependencies have the same values as before the go_transition, + # which can prevent unnecessary rebuilds caused by configuration + # changes. + # 2. Correctness in edge cases: If there is a path in the build graph + # from a go_binary's non-Go dependency to a go_library that does not + # pass through another go_binary (e.g., through a custom rule + # replacement for go_binary), this transition could be applied again + # and cause incorrect Go setting values. + new_settings[original_key] = "" + + 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(), +) + def _check_ternary(name, value): if value not in ("on", "off", "auto"): fail('{}: must be "on", "off", or "auto"'.format(name)) diff --git a/tests/core/transition/hermeticity_test.go b/tests/core/transition/hermeticity_test.go index 4c45c7cafa..136d5fdb4d 100644 --- a/tests/core/transition/hermeticity_test.go +++ b/tests/core/transition/hermeticity_test.go @@ -16,6 +16,7 @@ package hermeticity_test import ( "bytes" + "encoding/json" "fmt" "strings" "testing" @@ -27,9 +28,74 @@ func TestMain(m *testing.M) { bazel_testing.TestMain(m, bazel_testing.Args{ Main: ` -- BUILD.bazel -- +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test") load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") load("@rules_proto//proto:defs.bzl", "proto_library") +go_binary( + name = "main", + srcs = [ + "main.go", + ":gen_go", + ], + data = [":helper"], + embedsrcs = [":helper"], + cdeps = [":helper"], + cgo = True, + linkmode = "c-archive", + gotags = ["foo"], + deps = [":lib"], +) + +go_library( + name = "lib", + srcs = [ + "lib.go", + ":gen_indirect_go", + ], + importpath = "example.com/lib", + data = [":indirect_helper"], + embedsrcs = [":indirect_helper"], + cdeps = [":indirect_helper"], + cgo = True, +) + +go_test( + name = "main_test", + srcs = [ + "main.go", + ":gen_go", + ], + data = [":helper"], + embedsrcs = [":helper"], + cdeps = [":helper"], + cgo = True, + linkmode = "c-archive", + gotags = ["foo"], +) + +cc_library( + name = "helper", +) + +cc_library( + name = "indirect_helper", +) + +genrule( + name = "gen_go", + outs = ["gen.go"], + exec_tools = [":helper"], + cmd = "# Not needed for bazel cquery", +) + +genrule( + name = "gen_indirect_go", + outs = ["gen_indirect.go"], + exec_tools = [":indirect_helper"], + cmd = "# Not needed for bazel cquery", +) + proto_library( name = "foo_proto", srcs = ["foo.proto"], @@ -40,6 +106,11 @@ go_proto_library( importpath = "github.com/bazelbuild/rules_go/tests/core/transition/foo", proto = ":foo_proto", ) +-- main.go -- +package main + +func main() {} +-- lib.go -- -- foo.proto -- syntax = "proto3"; @@ -82,15 +153,32 @@ http_archive( }) } -func TestGoProtoLibraryToolAttrsAreReset(t *testing.T) { - assertDependsCleanlyOn(t, "//:foo_go_proto", "@com_google_protobuf//:protoc") +func TestGoBinaryNonGoAttrsAreReset(t *testing.T) { + assertDependsCleanlyOnWithFlags( + t, + "//:main", + "//:helper") } -func assertDependsCleanlyOn(t *testing.T, targetA, targetB string) { +func TestGoLibraryNonGoAttrsAreReset(t *testing.T) { assertDependsCleanlyOnWithFlags( t, - targetA, - targetB, + "//:main", + "//:indirect_helper") +} + +func TestGoTestNonGoAttrsAreReset(t *testing.T) { + assertDependsCleanlyOnWithFlags( + t, + "//:main_test", + "//:helper") +} + +func TestGoProtoLibraryToolAttrsAreReset(t *testing.T) { + assertDependsCleanlyOnWithFlags( + t, + "//:foo_go_proto", + "@com_google_protobuf//:protoc", "--@io_bazel_rules_go//go/config:static", "--@io_bazel_rules_go//go/config:msan", "--@io_bazel_rules_go//go/config:race", @@ -100,8 +188,8 @@ func assertDependsCleanlyOn(t *testing.T, targetA, targetB string) { ) assertDependsCleanlyOnWithFlags( t, - targetA, - targetB, + "//:foo_go_proto", + "@com_google_protobuf//:protoc", "--@io_bazel_rules_go//go/config:pure", ) } @@ -112,6 +200,7 @@ func assertDependsCleanlyOnWithFlags(t *testing.T, targetA, targetB string, flag []string{ "cquery", "--transitions=full", + "--output=jsonproto", query, }, flags..., @@ -120,15 +209,18 @@ func assertDependsCleanlyOnWithFlags(t *testing.T, targetA, targetB string, flag if err != nil { t.Fatalf("bazel cquery '%s': %v", query, err) } - cqueryOut := string(bytes.TrimSpace(out)) + cqueryOut := bytes.TrimSpace(out) configHashes := extractConfigHashes(t, cqueryOut) if len(configHashes) != 1 { - t.Fatalf( - "%s depends on %s in multiple configs with these differences in rules_go options: %s", - targetA, - targetB, - strings.Join(getGoOptions(t, configHashes...), "\n"), - ) + differingGoOptions := getGoOptions(t, configHashes...) + if len(differingGoOptions) != 0 { + t.Fatalf( + "%s depends on %s in multiple configs with these differences in rules_go options: %s", + targetA, + targetB, + strings.Join(differingGoOptions, "\n"), + ) + } } goOptions := getGoOptions(t, configHashes[0]) if len(goOptions) != 0 { @@ -141,36 +233,57 @@ func assertDependsCleanlyOnWithFlags(t *testing.T, targetA, targetB string, flag } } -func extractConfigHashes(t *testing.T, cqueryOut string) []string { - lines := strings.Split(cqueryOut, "\n") +func extractConfigHashes(t *testing.T, rawJsonOut []byte) []string { + var jsonOut bazelCqueryOutput + err := json.Unmarshal(rawJsonOut, &jsonOut) + if err != nil { + t.Fatalf("Failed to decode bazel config JSON output %v: %q", err, string(rawJsonOut)) + } var hashes []string - for _, line := range lines { - openingParens := strings.Index(line, "(") - closingParens := strings.Index(line, ")") - if openingParens == -1 || closingParens <= openingParens { - t.Fatalf("failed to find config hash in cquery out line: %s", line) - } - hashes = append(hashes, line[openingParens+1:closingParens]) + for _, result := range jsonOut.Results { + hashes = append(hashes, result.Configuration.Checksum) } return hashes } func getGoOptions(t *testing.T, hashes ...string) []string { - out, err := bazel_testing.BazelOutput(append([]string{"config"}, hashes...)...) + out, err := bazel_testing.BazelOutput(append([]string{"config", "--output=json"}, hashes...)...) if err != nil { t.Fatalf("bazel config %s: %v", strings.Join(hashes, " "), err) } - lines := strings.Split(string(bytes.TrimSpace(out)), "\n") - differingGoOptions := make([]string, 0) - for _, line := range lines { - // Lines with configuration options are indented - if !strings.HasPrefix(line, " ") { + rawJsonOut := bytes.TrimSpace(out) + var jsonOut bazelConfigOutput + err = json.Unmarshal(rawJsonOut, &jsonOut) + if err != nil { + t.Fatalf("Failed to decode bazel config JSON output %v: %q", err, string(rawJsonOut)) + } + var differingGoOptions []string + for _, fragment := range jsonOut.Fragments { + if fragment.Name != starlarkOptionsFragment { continue } - optionAndValue := strings.TrimLeft(line, " ") - if strings.HasPrefix(optionAndValue, "@io_bazel_rules_go//") { - differingGoOptions = append(differingGoOptions, optionAndValue) + for key, value := range fragment.Options { + if strings.HasPrefix(key, "@io_bazel_rules_go//") { + differingGoOptions = append(differingGoOptions, fmt.Sprintf("%s=%s", key, value)) + } } } return differingGoOptions } + +const starlarkOptionsFragment = "user-defined" + +type bazelConfigOutput struct { + Fragments []struct { + Name string `json:"name"` + Options map[string]string `json:"options"` + } `json:"fragments"` +} + +type bazelCqueryOutput struct { + Results []struct { + Configuration struct { + Checksum string `json:"checksum"` + } `json:"configuration"` + } `json:"results"` +}