From c92a0345f8298bb9b3f1a3ecedde17816c0bcfb7 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 16 Jun 2022 11:38:05 +0100 Subject: [PATCH] Review comments --- go/private/context.bzl | 10 +++-- go/private/rules/binary.bzl | 5 --- go/private/rules/library.bzl | 11 ----- go/private/rules/test.bzl | 5 --- go/private/rules/transition.bzl | 42 ------------------- go/private/rules/wrappers.bzl | 4 +- .../configurable_attribute_bad_test.go | 4 +- 7 files changed, 11 insertions(+), 70 deletions(-) diff --git a/go/private/context.bzl b/go/private/context.bzl index 0c2b11cdb8..054b86efff 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -392,7 +392,7 @@ def go_context(ctx, attr = None): coverdata = None nogo = None if hasattr(attr, "_go_context_data"): - go_context_data = _flatten(attr._go_context_data) + go_context_data = _flatten_possibly_transitioned_attr(attr._go_context_data) if CgoContextInfo in go_context_data: cgo_context_info = go_context_data[CgoContextInfo] go_config_info = go_context_data[GoConfigInfo] @@ -406,7 +406,7 @@ def go_context(ctx, attr = None): if hasattr(attr, "_go_config"): go_config_info = attr._go_config[GoConfigInfo] if hasattr(attr, "_stdlib"): - stdlib = _flatten(attr._stdlib)[GoStdLib] + stdlib = _flatten_possibly_transitioned_attr(attr._stdlib)[GoStdLib] mode = get_mode(ctx, toolchain, cgo_context_info, go_config_info) tags = mode.tags @@ -866,12 +866,14 @@ go_config = rule( def _expand_opts(go, attribute_name, opts): return [go._ctx.expand_make_variables(attribute_name, opt, {}) for opt in opts] +_LIST_TYPE = type([]) + # Used to get attribute values which may have been transitioned. # Transitioned attributes end up as lists. # We never use split-transitions, so we always expect exactly one element in those lists. # But if the attribute wasn't transitioned, it won't be a list. -def _flatten(maybe_list): - if type(maybe_list) == "list": +def _flatten_possibly_transitioned_attr(maybe_list): + if type(maybe_list) == _LIST_TYPE: if len(maybe_list) == 1: return maybe_list[0] else: diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index c6768a31d9..e1c923a508 100644 --- a/go/private/rules/binary.bzl +++ b/go/private/rules/binary.bzl @@ -34,7 +34,6 @@ load( load( "//go/private/rules:transition.bzl", "go_transition", - "non_go_transition", ) load( "//go/private:mode.bzl", @@ -189,7 +188,6 @@ _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, @@ -200,7 +198,6 @@ _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 @@ -230,7 +227,6 @@ _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` @@ -283,7 +279,6 @@ _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`. diff --git a/go/private/rules/library.bzl b/go/private/rules/library.bzl index 1a0a55733c..4afb629c6e 100644 --- a/go/private/rules/library.bzl +++ b/go/private/rules/library.bzl @@ -31,10 +31,6 @@ load( "GoLibrary", "INFERRED_PATH", ) -load( - "//go/private/rules:transition.bzl", - "non_go_transition", -) def _go_library_impl(ctx): """Implements the go_library() rule.""" @@ -63,7 +59,6 @@ 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. @@ -73,7 +68,6 @@ 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, @@ -116,7 +110,6 @@ 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. @@ -144,7 +137,6 @@ 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`. @@ -176,9 +168,6 @@ 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 = [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 4eae434201..fb1d413cc9 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -44,7 +44,6 @@ load( load( "//go/private/rules:transition.bzl", "go_transition", - "non_go_transition", ) load( "//go/private:mode.bzl", @@ -196,7 +195,6 @@ _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 @@ -206,7 +204,6 @@ _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, @@ -236,7 +233,6 @@ _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` @@ -309,7 +305,6 @@ _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`. diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl index f567028398..d08cb6f910 100644 --- a/go/private/rules/transition.bzl +++ b/go/private/rules/transition.bzl @@ -347,48 +347,6 @@ 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/go/private/rules/wrappers.bzl b/go/private/rules/wrappers.bzl index 7c4993d608..85fc848d6e 100644 --- a/go/private/rules/wrappers.bzl +++ b/go/private/rules/wrappers.bzl @@ -33,6 +33,8 @@ load( "go_test", ) +_SELECT_TYPE = type(select({"//conditions:default": ""})) + def _cgo(name, kwargs): if "objc" in kwargs: fail("//{}:{}: the objc attribute has been removed. .m sources may be included in srcs or may be extracted into a separated objc_library listed in cdeps.".format(native.package_name(), name)) @@ -48,7 +50,7 @@ def go_binary_macro(name, **kwargs): if kwargs.get("goos") != None or kwargs.get("goarch") != None: for key, value in kwargs.items(): - if type(value) == "select": + if type(value) == _SELECT_TYPE: # In the long term, we should replace goos/goarch with Bazel-native platform # support, but while we have the mechanisms, we try to avoid people trying to use # _both_ goos/goarch _and_ native platform support. diff --git a/tests/core/go_binary/configurable_attribute_bad_test.go b/tests/core/go_binary/configurable_attribute_bad_test.go index e043275d65..abd35ba29a 100644 --- a/tests/core/go_binary/configurable_attribute_bad_test.go +++ b/tests/core/go_binary/configurable_attribute_bad_test.go @@ -32,8 +32,8 @@ go_binary( srcs = [ "main.go", ], - goos = "darwin", - goarch = "amd64", + goos = "darwin", + goarch = "amd64", gotags = select({ "@io_bazel_rules_go//go/platform:linux": ["penguins"], "//conditions:default": ["nopenguins"],