From 56bbbb2653cf4ebb1e2c7915d9d330a7b4aac164 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 11 Apr 2022 17:05:58 +0100 Subject: [PATCH] Transition on edges not self As per https://github.com/bazelbuild/bazel/issues/15157 currently we can't configure any transition-relevant attributes using select. This is because we use self-transitions on targets, and when this happens, configurable attributes aren't passed to the transition, so we make incorrect transition decisions. Instead, insert a dummy rule which exists only to transition its dependencies using an edge transition, which means the appropriate attributes are resolved before the transition is called. To achieve this, we symlink the actual built binaries into our transitioning wrapper rules, and forward any providers needed. Fixes #3103. --- go/private/rules/BUILD.bazel | 3 + go/private/rules/binary.bzl | 44 +++++--- go/private/rules/test.bzl | 52 ++++++++-- go/private/rules/transition.bzl | 166 ++++++++++++++++++++++-------- go/private/rules/wrappers.bzl | 9 +- go/private/skylib/lib/BUILD.bazel | 6 ++ go/private/skylib/lib/dicts.bzl | 16 +++ 7 files changed, 225 insertions(+), 71 deletions(-) create mode 100644 go/private/skylib/lib/dicts.bzl diff --git a/go/private/rules/BUILD.bazel b/go/private/rules/BUILD.bazel index 2341744bfe..9fbbd4aeae 100644 --- a/go/private/rules/BUILD.bazel +++ b/go/private/rules/BUILD.bazel @@ -43,6 +43,7 @@ bzl_library( "//go/private:providers", "//go/private:rpath", "//go/private/rules:transition", + "@bazel_skylib//lib:dicts", ], ) @@ -132,6 +133,7 @@ bzl_library( "//go/private:providers", "//go/private/rules:binary", "//go/private/rules:transition", + "@bazel_skylib//lib:dicts", "@bazel_skylib//lib:structs", ], ) @@ -148,6 +150,7 @@ bzl_library( "//go/private:mode", "//go/private:platforms", "//go/private:providers", + "//go/private/skylib/lib:dicts", ], ) diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index 8d95020ef9..a72104b106 100644 --- a/go/private/rules/binary.bzl +++ b/go/private/rules/binary.bzl @@ -29,8 +29,11 @@ load( ) load( "//go/private/rules:transition.bzl", - "go_transition_rule", "non_go_transition", + "ForwardingPastTransitionProvider", + "forward_through_transition_impl", + "go_transition", + "transition_attrs", ) load( "//go/private:mode.bzl", @@ -44,6 +47,10 @@ load( "//go/private:rpath.bzl", "rpath", ) +load( + "@bazel_skylib//lib:dicts.bzl", + "dicts", +) _EMPTY_DEPSET = depset([]) @@ -124,7 +131,7 @@ def _go_binary_impl(ctx): executable = executable, ) - providers = [ + providers_to_forward = [ library, source, archive, @@ -132,11 +139,6 @@ def _go_binary_impl(ctx): cgo_exports = archive.cgo_exports, compilation_outputs = [archive.data.file], ), - DefaultInfo( - files = depset([executable]), - runfiles = runfiles, - executable = executable, - ), ] # If the binary's linkmode is c-archive or c-shared, expose CcInfo @@ -165,11 +167,19 @@ def _go_binary_impl(ctx): ccinfo = cc_common.merge_cc_infos( cc_infos = [ccinfo] + [d[CcInfo] for d in source.cdeps], ) - providers.append(ccinfo) + providers_to_forward.append(ccinfo) - return providers + forwarding_provider = ForwardingPastTransitionProvider(executable = executable, runfiles = runfiles, providers_to_forward = providers_to_forward) + return providers_to_forward + [ + forwarding_provider, + DefaultInfo( + files = depset([executable]), + runfiles = runfiles, + executable = executable, + ), + ] -_go_binary_kwargs = { +go_binary_kwargs = { "implementation": _go_binary_impl, "attrs": { "srcs": attr.label_list( @@ -394,9 +404,17 @@ _go_binary_kwargs = { """, } -go_binary = rule(executable = True, **_go_binary_kwargs) -go_transition_binary = go_transition_rule(executable = True, **_go_binary_kwargs) -go_non_executable_transition_binary = go_transition_rule(executable = False, **_go_binary_kwargs) +go_binary = rule(executable = True, **go_binary_kwargs) +go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs) + +go_transition_binary = rule( + implementation = forward_through_transition_impl, + attrs = dicts.add(transition_attrs, { + "is_windows": attr.bool(), + "transition_dep": attr.label(cfg = go_transition), + }), + executable = True, +) def _go_tool_binary_impl(ctx): sdk = ctx.attr.sdk[GoSDK] diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl index 7e860d18e4..c7ef4d14de 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -39,13 +39,20 @@ load( ) load( "//go/private/rules:transition.bzl", - "go_transition_rule", "non_go_transition", + "ForwardingPastTransitionProvider", + "forward_through_transition_impl", + "go_transition", + "transition_attrs", ) load( "//go/private:mode.bzl", "LINKMODE_NORMAL", ) +load( + "@bazel_skylib//lib:dicts.bzl", + "dicts", +) load( "@bazel_skylib//lib:structs.bzl", "structs", @@ -54,6 +61,22 @@ load( def _testmain_library_to_source(go, attr, source, merge): source["deps"] = source["deps"] + [attr.library] +go_transition_test = rule( + implementation = forward_through_transition_impl, + attrs = dicts.add(transition_attrs, { + "transition_dep": attr.label(cfg = go_transition), + "is_windows": attr.bool(), + # Workaround for bazelbuild/bazel#6293. See comment in lcov_merger.sh. + "_lcov_merger": attr.label( + executable = True, + default = "//go/tools/builders:lcov_merger", + cfg = "target", + ), + }), + executable = True, + test = True, +) + def _go_test_impl(ctx): """go_test_impl implements go testing. @@ -168,13 +191,8 @@ def _go_test_impl(ctx): # source file is present, Bazel will set the COVERAGE_OUTPUT_FILE # environment variable during tests and will save that file to the build # events + test outputs. - return [ + providers_to_forward = [ test_archive, - DefaultInfo( - files = depset([executable]), - runfiles = runfiles, - executable = executable, - ), OutputGroupInfo( compilation_outputs = [internal_archive.data.file], ), @@ -187,7 +205,22 @@ def _go_test_impl(ctx): testing.TestEnvironment(env), ] -_go_test_kwargs = { + forwarding_provider = ForwardingPastTransitionProvider( + executable = executable, + runfiles = runfiles, + providers_to_forward = providers_to_forward, + ) + + return providers_to_forward + [ + forwarding_provider, + DefaultInfo( + files = depset([executable]), + runfiles = runfiles, + executable = executable, + ), + ] + +go_test_kwargs = { "implementation": _go_test_impl, "attrs": { "data": attr.label_list( @@ -448,8 +481,7 @@ _go_test_kwargs = { """, } -go_test = rule(**_go_test_kwargs) -go_transition_test = go_transition_rule(**_go_test_kwargs) +go_test = rule(**go_test_kwargs) def _recompile_external_deps(go, external_source, internal_archive, library_labels): """Recompiles some archives in order to split internal and external tests. diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl index 6499caecfc..d878e16d70 100644 --- a/go/private/rules/transition.bzl +++ b/go/private/rules/transition.bzl @@ -36,6 +36,10 @@ load( "//go/platform:crosstool.bzl", "platform_from_crosstool", ) +load( + "//go/private/skylib/lib:dicts.bzl", + "dicts", +) def filter_transition_label(label): """Transforms transition labels for the current workspace. @@ -80,7 +84,24 @@ _SETTING_KEY_TO_ORIGINAL_SETTING_KEY = { for setting in TRANSITIONED_GO_SETTING_KEYS } -def go_transition_wrapper(kind, transition_kind, name, **kwargs): +ForwardingPastTransitionProvider = provider( + """Provider for a go_transition_wrapper to forward information from its dep. + + go_transition_wrapper symlinks outputs from some rule, after performing a + transition on the rule it depends on. It also forwards all of the providers + from that rule. + + This provider allows structured passing of the required data in order to + perform that forwarding. + """, + fields = { + "executable": "File: The executable of the dep, which should be symlinked to.", + "runfiles": "Runfiles: The runfiles of the dep.", + "providers_to_forward": "[Provider]: The providers the transition-wrapping rule should expose.", + }, +) + +def go_transition_wrapper(kind, transition_kind, name, use_basename, keys_to_strip, **kwargs): """Wrapper for rules that may use transitions. This is used in place of instantiating go_binary or go_transition_binary @@ -90,51 +111,71 @@ def go_transition_wrapper(kind, transition_kind, name, **kwargs): configuration identical to the default configuration. """ transition_keys = ("goos", "goarch", "pure", "static", "msan", "race", "gotags", "linkmode") + keys_to_strip = [k for k in keys_to_strip if k not in transition_keys] need_transition = any([key in kwargs for key in transition_keys]) if need_transition: - transition_kind(name = name, **kwargs) + transitioned_name = name + ".transitioned" + + # Strip out attrs that are expected by the underlying rule. + # This means that we strip out things like srcs, because we don't need them, + # but preserve things like tags and exec_compatible_with, + # which are general to all tests rather than specific to go_test specifically, + # and should apply to generated tests and binaries too. + transition_kwargs = dicts.omit(kwargs, keys_to_strip) + transition_kwargs["name"] = name + transition_kwargs["transition_dep"] = transitioned_name + transition_kwargs["is_windows"] = select({ + "@bazel_tools//src/conditions:windows": True, + "//conditions:default": False, + }) + transition_kind(**transition_kwargs) + + tags = kwargs.pop("tags", []) + if "manual" not in tags: + tags += ["manual"] + kwargs["tags"] = tags + + if use_basename and "basename" not in kwargs: + kwargs["basename"] = name + + kind(name = transitioned_name, **kwargs) else: kind(name = name, **kwargs) -def go_transition_rule(**kwargs): - """Like "rule", but adds a transition and mode attributes.""" - kwargs = dict(kwargs) - kwargs["attrs"].update({ - "goos": attr.string( - default = "auto", - values = ["auto"] + {goos: None for goos, _ in GOOS_GOARCH}.keys(), - ), - "goarch": attr.string( - default = "auto", - values = ["auto"] + {goarch: None for _, goarch in GOOS_GOARCH}.keys(), - ), - "pure": attr.string( - default = "auto", - values = ["auto", "on", "off"], - ), - "static": attr.string( - default = "auto", - values = ["auto", "on", "off"], - ), - "msan": attr.string( - default = "auto", - values = ["auto", "on", "off"], - ), - "race": attr.string( - default = "auto", - values = ["auto", "on", "off"], - ), - "gotags": attr.string_list(default = []), - "linkmode": attr.string( - default = "auto", - values = ["auto"] + LINKMODES, - ), - "_whitelist_function_transition": attr.label( - default = "@bazel_tools//tools/whitelists/function_transition_whitelist", - ), - }) - kwargs["cfg"] = go_transition - return rule(**kwargs) +transition_attrs = { + "goos": attr.string( + default = "auto", + values = ["auto"] + {goos: None for goos, _ in GOOS_GOARCH}.keys(), + ), + "goarch": attr.string( + default = "auto", + values = ["auto"] + {goarch: None for _, goarch in GOOS_GOARCH}.keys(), + ), + "pure": attr.string( + default = "auto", + values = ["auto", "on", "off"], + ), + "static": attr.string( + default = "auto", + values = ["auto", "on", "off"], + ), + "msan": attr.string( + default = "auto", + values = ["auto", "on", "off"], + ), + "race": attr.string( + default = "auto", + values = ["auto", "on", "off"], + ), + "gotags": attr.string_list(default = []), + "linkmode": attr.string( + default = "auto", + values = ["auto"] + LINKMODES, + ), + "_whitelist_function_transition": attr.label( + default = "@bazel_tools//tools/whitelists/function_transition_whitelist", + ), +} def _go_transition_impl(settings, attr): # NOTE: Keep the list of rules_go settings set by this transition in sync @@ -333,7 +374,7 @@ def _go_reset_target_impl(ctx): new_executable = None original_executable = default_info.files_to_run.executable - default_runfiles = default_info.default_runfiles + runfiles_wrapper = default_info if original_executable: # In order for the symlink to have the same basename as the original # executable (important in the case of proto plugins), put it in a @@ -344,13 +385,13 @@ def _go_reset_target_impl(ctx): target_file = original_executable, is_executable = True, ) - default_runfiles = default_runfiles.merge(ctx.runfiles([new_executable])) + runfiles_wrapper = _add_to_runfiles(ctx, runfiles_wrapper, new_executable) providers.append( DefaultInfo( files = default_info.files, - data_runfiles = default_info.data_runfiles, - default_runfiles = default_runfiles, + data_runfiles = runfiles_wrapper.data_runfiles, + default_runfiles = runfiles_wrapper.default_runfiles, executable = new_executable, ), ) @@ -456,3 +497,38 @@ def _set_ternary(settings, attr, name): label = filter_transition_label("@io_bazel_rules_go//go/config:{}".format(name)) settings[label] = value == "on" return value + +def _symlink_file_to_rule_name(ctx, src): + output_file_name = ctx.label.name + (".exe" if ctx.attr.is_windows else "") + dst = ctx.actions.declare_file(paths.join(ctx.label.name, output_file_name)) + ctx.actions.symlink( + output = dst, + target_file = src, + is_executable = True, + ) + return dst + +def _add_to_runfiles(ctx, default_info, file): + if file == None: + return default_info + + data_runfiles = default_info.data_runfiles.merge(ctx.runfiles([file])) + default_runfiles = default_info.data_runfiles.merge(ctx.runfiles([file])) + return struct( + data_runfiles = data_runfiles, + default_runfiles = default_runfiles, + ) + +def forward_through_transition_impl(ctx): + forwarding_provider = ctx.attr.transition_dep[0][ForwardingPastTransitionProvider] + default_info = ctx.attr.transition_dep[0][DefaultInfo] + + copied_executable = _symlink_file_to_rule_name(ctx, forwarding_provider.executable) + runfiles = _add_to_runfiles(ctx, default_info, copied_executable) + + return [DefaultInfo( + executable = copied_executable, + files = default_info.files, + data_runfiles = runfiles.data_runfiles, + default_runfiles = runfiles.default_runfiles, + )] + forwarding_provider.providers_to_forward diff --git a/go/private/rules/wrappers.bzl b/go/private/rules/wrappers.bzl index df7ccec78f..4d98111094 100644 --- a/go/private/rules/wrappers.bzl +++ b/go/private/rules/wrappers.bzl @@ -27,11 +27,13 @@ load( "//go/private/rules:binary.bzl", "go_binary", "go_non_executable_transition_binary", + "go_binary_kwargs", "go_transition_binary", ) load( "//go/private/rules:test.bzl", "go_test", + "go_test_kwargs", "go_transition_test", ) load( @@ -51,12 +53,13 @@ def go_library_macro(name, **kwargs): def go_binary_macro(name, **kwargs): """See docs/go/core/rules.md#go_binary for full documentation.""" _cgo(name, kwargs) + go_binary_attr_keys = go_binary_kwargs["attrs"].keys() if kwargs.get("linkmode", default = LINKMODE_NORMAL) in LINKMODES_EXECUTABLE: - go_transition_wrapper(go_binary, go_transition_binary, name = name, **kwargs) + go_transition_wrapper(go_binary, go_transition_binary, name = name, use_basename = True, keys_to_strip = go_binary_attr_keys, **kwargs) else: # A non-normal link mode implies the use of transitions, so we don't have to define a # non-executable version of the untransitioned go_binary. - go_transition_wrapper(None, go_non_executable_transition_binary, name = name, **kwargs) + go_non_executable_transition_binary(name = name, **kwargs) if kwargs.get("linkmode") in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED): # Create an alias to tell users of the `.cc` rule that it is deprecated. native.alias( @@ -70,4 +73,4 @@ def go_binary_macro(name, **kwargs): def go_test_macro(name, **kwargs): """See docs/go/core/rules.md#go_test for full documentation.""" _cgo(name, kwargs) - go_transition_wrapper(go_test, go_transition_test, name = name, **kwargs) + go_transition_wrapper(go_test, go_transition_test, name = name, use_basename = False, keys_to_strip = go_test_kwargs["attrs"].keys(), **kwargs) diff --git a/go/private/skylib/lib/BUILD.bazel b/go/private/skylib/lib/BUILD.bazel index c7cd85c778..f36c887c3c 100644 --- a/go/private/skylib/lib/BUILD.bazel +++ b/go/private/skylib/lib/BUILD.bazel @@ -18,3 +18,9 @@ bzl_library( srcs = ["versions.bzl"], visibility = ["//go:__subpackages__"], ) + +bzl_library( + name = "dicts", + srcs = ["dicts.bzl"], + visibility = ["//go:__subpackages__"], +) diff --git a/go/private/skylib/lib/dicts.bzl b/go/private/skylib/lib/dicts.bzl new file mode 100644 index 0000000000..c0276936a0 --- /dev/null +++ b/go/private/skylib/lib/dicts.bzl @@ -0,0 +1,16 @@ +# This function isn't yet in a released skylib. It's taken from https://github.com/bazelbuild/bazel-skylib/blob/8334f938c1574ef6f1f7a38a03550a31df65274e/lib/dicts.bzl + +def _omit(dictionary, keys): + """Returns a new `dict` that has all the entries of `dictionary` with keys not in `keys`. + Args: + dictionary: A `dict`. + keys: A sequence. + Returns: + A new `dict` that has all the entries of `dictionary` with keys not in `keys`. + """ + keys_set = {k: None for k in keys} + return {k: dictionary[k] for k in dictionary if k not in keys_set} + +dicts = struct( + omit = _omit, +)