Reset Go settings for protoc dependencies (#3005)

go/private/rules/nogo.bzl

@@ -25,7 +25,7 @@ load(
 )
 load(
     "//go/private/rules:transition.bzl",
-    "go_reset_transition",
+    "go_tool_transition",
 )
 
 def _nogo_impl(ctx):
@@ -103,7 +103,7 @@ _nogo = rule(
         ),
     },
     toolchains = ["@io_bazel_rules_go//go:toolchain"],
-    cfg = go_reset_transition,
+    cfg = go_tool_transition,
 )
 
 def nogo(name, visibility = None, **kwargs):

go/private/rules/transition.bzl

@@ -221,7 +221,7 @@ go_transition = transition(
     ]],
 )
 
-_reset_transition_dict = {
+_common_reset_transition_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,17 +230,22 @@ _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": [],
-    "@io_bazel_rules_go//go/private:bootstrap_nogo": True,
 }
 
+_reset_transition_dict = dict(_common_reset_transition_dict, **{
+    "@io_bazel_rules_go//go/private:bootstrap_nogo": True,
+})
+
 _reset_transition_keys = sorted([filter_transition_label(label) for label in _reset_transition_dict.keys()])
 
-def _go_reset_transition_impl(settings, attr):
-    """Sets Go settings to default values so tools can be built safely.
+def _go_tool_transition_impl(settings, attr):
+    """Sets most Go settings to default values (use for external Go tools).
 
-    go_reset_transition sets all of the //go/config settings to their default
-    values. This is used for tool binaries like nogo. Tool binaries shouldn't
-    depend on the link mode or tags of the target configuration. This transition
+    go_tool_transition sets all of the //go/config settings to their default
+    values and disables nogo. This is used for Go tool binaries like nogo
+    itself. Tool binaries shouldn't depend on the link mode or tags of the
+    target configuration and neither the tools nor the code they potentially
+    generate should be subject to nogo's static analysis. This transition
     doesn't change the platform (goos, goarch), but tool binaries should also
     have `cfg = "exec"` so tool binaries should be built for the execution
     platform.
@@ -250,15 +255,39 @@ def _go_reset_transition_impl(settings, attr):
         settings[filter_transition_label(label)] = value
     return settings
 
-go_reset_transition = transition(
-    implementation = _go_reset_transition_impl,
+go_tool_transition = transition(
+    implementation = _go_tool_transition_impl,
+    inputs = _reset_transition_keys,
+    outputs = _reset_transition_keys,
+)
+
+def _non_go_tool_transition_impl(settings, attr):
+    """Sets all Go settings to default values (use for external non-Go tools).
+
+    non_go_tool_transition sets all of the //go/config settings as well as the
+    nogo settings to their default values. This is used for all tools that are
+    not themselves targets created from rules_go rules and thus do not read
+    these settings. Resetting all of them to defaults prevents unnecessary
+    configuration changes for these targets that could cause rebuilds.
+
+    Examples: This transition is applied to attributes referencing proto_library
+    targets or protoc directly.
+    """
+    settings = dict(settings)
+    for label, value in _reset_transition_dict.items():
+        settings[filter_transition_label(label)] = value
+    settings[filter_transition_label("@io_bazel_rules_go//go/private:bootstrap_nogo")] = False
+    return settings
+
+non_go_tool_transition = transition(
+    implementation = _non_go_tool_transition_impl,
     inputs = _reset_transition_keys,
     outputs = _reset_transition_keys,
 )
 
 def _go_reset_target_impl(ctx):
     t = ctx.attr.dep[0]  # [0] seems to be necessary with the transition
-    providers = [t[p] for p in [GoLibrary, GoSource, GoArchive]]
+    providers = [t[p] for p in [GoLibrary, GoSource, GoArchive] if p in t]
 
     # We can't pass DefaultInfo through as-is, since Bazel forbids executable
     # if it's a file declared in a different target. To emulate that, symlink
@@ -295,22 +324,45 @@ go_reset_target = rule(
     attrs = {
         "dep": attr.label(
             mandatory = True,
-            cfg = go_reset_transition,
+            cfg = go_tool_transition,
         ),
-        "_whitelist_function_transition": attr.label(
-            default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
+        "_allowlist_function_transition": attr.label(
+            default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
         ),
     },
-    doc = """Forwards providers from a target and applies go_reset_transition.
+    doc = """Forwards providers from a target and applies go_tool_transition.
 
-go_reset_target depends on a single target, built using go_reset_transition.
-It forwards Go providers and DefaultInfo.
+go_reset_target depends on a single target, built using go_tool_transition. It
+forwards Go providers and DefaultInfo.
 
-This is used to work around a problem with building tools: tools should be
+This is used to work around a problem with building tools: Go tools should be
 built with 'cfg = "exec"' so they work on the execution platform, but we also
-need to apply go_reset_transition, so for example, a tool isn't built as a
-shared library with race instrumentation. This acts as an intermediately rule
-so we can apply both transitions.
+need to apply go_tool_transition so that e.g. a tool isn't built as a shared
+library with race instrumentation. This acts as an intermediate rule that allows
+to apply both both transitions.
+""",
+)
+
+non_go_reset_target = rule(
+    implementation = _go_reset_target_impl,
+    attrs = {
+        "dep": attr.label(
+            mandatory = True,
+            cfg = non_go_tool_transition,
+        ),
+        "_allowlist_function_transition": attr.label(
+            default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
+        ),
+    },
+    doc = """Forwards providers from a target and applies non_go_tool_transition.
+
+non_go_reset_target depends on a single target, built using
+non_go_tool_transition. It forwards Go providers and DefaultInfo.
+
+This is used to work around a problem with building tools: Non-Go tools should
+be built with 'cfg = "exec"' so they work on the execution platform, but they
+also shouldn't be affected by Go-specific config changes applied by
+go_transition.
 """,
 )
 

proto/BUILD.bazel

@@ -3,6 +3,10 @@ load(
     "//proto:compiler.bzl",
     "go_proto_compiler",
 )
+load(
+    "//go/private/rules:transition.bzl",
+    "non_go_reset_target",
+)
 load(
     "//proto/wkt:well_known_types.bzl",
     "GOGO_WELL_KNOWN_TYPE_REMAPS",
@@ -112,6 +116,12 @@ go_proto_compiler(
     ] + WELL_KNOWN_TYPE_RULES.values(),
 )
 
+non_go_reset_target(
+    name = "protoc",
+    dep = "@com_google_protobuf//:protoc",
+    visibility = ["//visibility:public"],
+)
+
 filegroup(
     name = "all_rules",
     testonly = True,

proto/compiler.bzl

@@ -200,7 +200,7 @@ _go_proto_compiler = rule(
         "_protoc": attr.label(
             executable = True,
             cfg = "exec",
-            default = "@com_google_protobuf//:protoc",
+            default = "//proto:protoc",
         ),
         "_go_context_data": attr.label(
             default = "//:go_context_data",

proto/def.bzl

@@ -18,6 +18,10 @@ load(
     "GoSource",
     "go_context",
 )
+load(
+    "@bazel_skylib//lib:types.bzl",
+    "types",
+)
 load(
     "//proto:compiler.bzl",
     "GoProtoCompiler",
@@ -27,6 +31,10 @@ load(
     "//go/private:providers.bzl",
     "INFERRED_PATH",
 )
+load(
+    "//go/private/rules:transition.bzl",
+    "non_go_tool_transition",
+)
 load(
     "@rules_proto//proto:defs.bzl",
     "ProtoInfo",
@@ -37,8 +45,9 @@ GoProtoImports = provider()
 def get_imports(attr):
     proto_deps = []
 
-    if hasattr(attr, "proto") and attr.proto and ProtoInfo in attr.proto:
-        proto_deps = [attr.proto]
+    # ctx.attr.proto is a one-element array since there is a Starlark transition attached to it.
+    if hasattr(attr, "proto") and attr.proto and types.is_list(attr.proto) and ProtoInfo in attr.proto[0]:
+        proto_deps = [attr.proto[0]]
     elif hasattr(attr, "protos"):
         proto_deps = [d for d in attr.protos if ProtoInfo in d]
     else:
@@ -92,7 +101,9 @@ def _go_proto_library_impl(ctx):
         #TODO: print("DEPRECATED: proto attribute on {}, use protos instead".format(ctx.label))
         if ctx.attr.protos:
             fail("Either proto or protos (non-empty) argument must be specified, but not both")
-        proto_deps = [ctx.attr.proto]
+
+        # ctx.attr.proto is a one-element array since there is a Starlark transition attached to it.
+        proto_deps = [ctx.attr.proto[0]]
     else:
         if not ctx.attr.protos:
             fail("Either proto or protos (non-empty) argument must be specified")
@@ -137,8 +148,12 @@ def _go_proto_library_impl(ctx):
 go_proto_library = rule(
     implementation = _go_proto_library_impl,
     attrs = {
-        "proto": attr.label(providers = [ProtoInfo]),
+        "proto": attr.label(
+            cfg = non_go_tool_transition,
+            providers = [ProtoInfo],
+        ),
         "protos": attr.label_list(
+            cfg = non_go_tool_transition,
             providers = [ProtoInfo],
             default = [],
         ),
@@ -159,6 +174,9 @@ go_proto_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"],
 )

tests/core/transition/BUILD.bazel

@@ -5,3 +5,9 @@ go_bazel_test(
     size = "medium",
     srcs = ["cmdline_test.go"],
 )
+
+go_bazel_test(
+    name = "hermeticity_test",
+    size = "medium",
+    srcs = ["hermeticity_test.go"],
+)