From d435c6dd6e977a5c3ea1bc726557a9321948a317 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 21 Sep 2023 07:05:40 -0700 Subject: [PATCH] Use proto_toolchain in proto_library Change --incompatible_enable_proto_toolchain_resolution flag to a loading time flag. This is partial revert of https://github.com/bazelbuild/bazel/commit/11cf1b756b77c6159fbb5c9ab480b742ea85b5b6. We need a load time flag, because toolchain type might not exist at current versions of rules repos users use. When the flag is set, proto_library defines the toolchain and doesn't use an implicit dependency at the same time. In Bazel the flag may only be flipped after all the rules_* repo are upgraded to define toolchain type and protobuf repository registers the toolchains. Issue: https://github.com/bazelbuild/rules_proto/issues/179 PiperOrigin-RevId: 567296665 Change-Id: Ib3fc27751dd14589fa403f45d2cbbad22537b9a3 --- .../semantics/BuildLanguageOptions.java | 17 +++++ .../devtools/build/lib/rules/proto/BUILD | 3 +- .../lib/rules/proto/BazelProtoCommon.java | 17 +++++ .../lib/rules/proto/ProtoConfiguration.java | 44 ++--------- .../common/proto/proto_library.bzl | 31 +++++--- .../common/proto/proto_semantics.bzl | 11 +++ .../lib/packages/util/MockProtoSupport.java | 75 +++++++++++++++++-- .../StarlarkJavaLiteProtoLibraryTest.java | 1 + .../rules/proto/BazelProtoLibraryTest.java | 15 ++++ .../build/lib/testutil/TestConstants.java | 3 + 10 files changed, 159 insertions(+), 58 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index ba12602767ae44..3e47365aa22119 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -712,6 +712,18 @@ public final class BuildLanguageOptions extends OptionsBase { help = "If enabled, targets that have unknown attributes set to None fail.") public boolean incompatibleFailOnUnknownAttributes; + // Flip when dependencies to rules_* repos are upgraded and protobuf registers toolchains + @Option( + name = "incompatible_enable_proto_toolchain_resolution", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If true, proto lang rules define toolchains from rules_proto, rules_java, rules_cc" + + " repositories.") + public boolean incompatibleEnableProtoToolchainResolution; + /** * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should * never accumulate a large number of these and being able to shortcut on object identity makes a @@ -813,6 +825,9 @@ public StarlarkSemantics toStarlarkSemantics() { INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION, incompatibleDisableObjcLibraryTransition) .setBool(INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES, incompatibleFailOnUnknownAttributes) + .setBool( + INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION, + incompatibleEnableProtoToolchainResolution) .build(); return INTERNER.intern(semantics); } @@ -906,6 +921,8 @@ public StarlarkSemantics toStarlarkSemantics() { "-incompatible_disable_objc_library_transition"; public static final String INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES = "+incompatible_fail_on_unknown_attributes"; + public static final String INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION = + "-incompatible_enable_proto_toolchain_resolution"; // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD b/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD index 83f837b4228573..a9b76cdd197785 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD @@ -32,7 +32,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", - "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations", @@ -40,10 +39,10 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/proto", "//src/main/java/com/google/devtools/build/lib/util:filetype", - "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/annot", "//src/main/java/net/starlark/java/eval", diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java index ac9b40f961f146..c1c7d71fbe5430 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java @@ -14,11 +14,28 @@ package com.google.devtools.build.lib.rules.proto; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.packages.BuiltinRestriction; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.starlarkbuildapi.proto.ProtoCommonApi; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkThread; /** Protocol buffers support for Starlark. */ public class BazelProtoCommon implements ProtoCommonApi { public static final BazelProtoCommon INSTANCE = new BazelProtoCommon(); protected BazelProtoCommon() {} + + @StarlarkMethod( + name = "incompatible_enable_proto_toolchain_resolution", + useStarlarkThread = true, + documented = false) + public boolean getDefineProtoToolchains(StarlarkThread thread) throws EvalException { + BuiltinRestriction.failIfCalledOutsideAllowlist(thread, ImmutableSet.of()); + return thread + .getSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION); + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java index 4ceab303f57ff5..7ec213474b484a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java @@ -46,17 +46,6 @@ public class ProtoConfiguration extends Fragment implements ProtoConfigurationAp /** Command line options. */ public static class Options extends FragmentOptions { - @Option( - name = "incompatible_enable_proto_toolchain_resolution", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, - metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - help = - "If true, proto lang rules use toolchain resolution to find the toolchain. The flags" - + " proto_compiler and proto_toolchain_for_* are a no-op.") - public boolean enableProtoToolchainResolution; - @Option( name = "protocopt", allowMultiple = true, @@ -175,7 +164,6 @@ public static class Options extends FragmentOptions { @Override public FragmentOptions getExec() { Options exec = (Options) super.getExec(); - exec.enableProtoToolchainResolution = enableProtoToolchainResolution; exec.protoCompiler = protoCompiler; exec.protocOpts = protocOpts; exec.experimentalProtoDescriptorSetsIncludeSourceInfo = @@ -196,12 +184,10 @@ public FragmentOptions getExec() { private final ImmutableList protocOpts; private final ImmutableList ccProtoLibraryHeaderSuffixes; private final ImmutableList ccProtoLibrarySourceSuffixes; - private final boolean enableProtoToolchainResolution; private final Options options; public ProtoConfiguration(BuildOptions buildOptions) { Options options = buildOptions.get(Options.class); - this.enableProtoToolchainResolution = options.enableProtoToolchainResolution; this.protocOpts = ImmutableList.copyOf(options.protocOpts); this.ccProtoLibraryHeaderSuffixes = ImmutableList.copyOf(options.ccProtoLibraryHeaderSuffixes); this.ccProtoLibrarySourceSuffixes = ImmutableList.copyOf(options.ccProtoLibrarySourceSuffixes); @@ -246,11 +232,7 @@ public boolean runExperimentalProtoExtraActions() { defaultLabel = ProtoConstants.DEFAULT_PROTOC_LABEL) @Nullable public Label protoCompiler() { - if (enableProtoToolchainResolution) { - return null; - } else { - return options.protoCompiler; - } + return options.protoCompiler; } @StarlarkConfigurationField( @@ -259,11 +241,7 @@ public Label protoCompiler() { defaultLabel = ProtoConstants.DEFAULT_JAVA_PROTO_LABEL) @Nullable public Label protoToolchainForJava() { - if (enableProtoToolchainResolution) { - return null; - } else { - return options.protoToolchainForJava; - } + return options.protoToolchainForJava; } @StarlarkConfigurationField( @@ -272,11 +250,7 @@ public Label protoToolchainForJava() { defaultLabel = ProtoConstants.DEFAULT_J2OBJC_PROTO_LABEL) @Nullable public Label protoToolchainForJ2objc() { - if (enableProtoToolchainResolution) { - return null; - } else { - return options.protoToolchainForJ2objc; - } + return options.protoToolchainForJ2objc; } @StarlarkConfigurationField( @@ -285,11 +259,7 @@ public Label protoToolchainForJ2objc() { defaultLabel = ProtoConstants.DEFAULT_JAVA_LITE_PROTO_LABEL) @Nullable public Label protoToolchainForJavaLite() { - if (enableProtoToolchainResolution) { - return null; - } else { - return options.protoToolchainForJavaLite; - } + return options.protoToolchainForJavaLite; } @StarlarkConfigurationField( @@ -298,11 +268,7 @@ public Label protoToolchainForJavaLite() { defaultLabel = ProtoConstants.DEFAULT_CC_PROTO_LABEL) @Nullable public Label protoToolchainForCc() { - if (enableProtoToolchainResolution) { - return null; - } else { - return options.protoToolchainForCc; - } + return options.protoToolchainForCc; } @StarlarkMethod(name = "strict_proto_deps", useStarlarkThread = true, documented = false) diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl index ae72b9a5083b80..2d8c805afb6ce2 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -208,15 +208,22 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set): map_each = proto_common.get_import_path, join_with = ":", ) - proto_lang_toolchain_info = proto_common.ProtoLangToolchainInfo( - out_replacement_format_flag = "--descriptor_set_out=%s", - output_files = "single", - mnemonic = "GenProtoDescriptorSet", - progress_message = "Generating Descriptor Set proto_library %{label}", - proto_compiler = ctx.executable._proto_compiler, - protoc_opts = ctx.fragments.proto.experimental_protoc_opts, - plugin = None, - ) + if semantics.INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION: + toolchain = ctx.toolchains[semantics.PROTO_TOOLCHAIN_TYPE] + if not toolchain: + fail("Protocol compiler toolchain could not be resolved.") + proto_lang_toolchain_info = toolchain.proto + else: + proto_lang_toolchain_info = proto_common.ProtoLangToolchainInfo( + out_replacement_format_flag = "--descriptor_set_out=%s", + output_files = "single", + mnemonic = "GenProtoDescriptorSet", + progress_message = "Generating Descriptor Set proto_library %{label}", + proto_compiler = ctx.executable._proto_compiler, + protoc_opts = ctx.fragments.proto.experimental_protoc_opts, + plugin = None, + ) + proto_common.compile( ctx.actions, proto_info, @@ -228,7 +235,7 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set): proto_library = rule( _proto_library_impl, - attrs = dict({ + attrs = { "srcs": attr.label_list( allow_files = [".proto", ".protodevel"], flags = ["DIRECT_COMPILE_TIME_INPUT"], @@ -249,14 +256,16 @@ proto_library = rule( flags = ["SKIP_CONSTRAINTS_OVERRIDE"], ), "licenses": attr.license() if hasattr(attr, "license") else attr.string_list(), + } | ({} if semantics.INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION else { "_proto_compiler": attr.label( cfg = "exec", executable = True, allow_files = True, default = configuration_field("proto", "proto_compiler"), ), - }, **semantics.EXTRA_ATTRIBUTES), + }) | semantics.EXTRA_ATTRIBUTES, fragments = ["proto"] + semantics.EXTRA_FRAGMENTS, provides = [ProtoInfo], exec_groups = semantics.EXEC_GROUPS, + toolchains = semantics.PROTO_TOOLCHAIN, ) diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl index d0aa27b32c400f..a9c931f22a279d 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl @@ -19,7 +19,18 @@ Proto Semantics def _preprocess(ctx): pass +_PROTO_TOOLCHAIN_TYPE = "@rules_proto//proto:toolchain_type" + +def _get_proto_toolchain(): + if _builtins.toplevel.proto_common.incompatible_enable_proto_toolchain_resolution(): + return [_builtins.toplevel.config_common.toolchain_type(_PROTO_TOOLCHAIN_TYPE, mandatory = False)] + else: + return [] + semantics = struct( + PROTO_TOOLCHAIN_TYPE = _PROTO_TOOLCHAIN_TYPE, + PROTO_TOOLCHAIN = _get_proto_toolchain(), + INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION = _builtins.toplevel.proto_common.incompatible_enable_proto_toolchain_resolution(), PROTO_COMPILER_LABEL = "@bazel_tools//tools/proto:protoc", EXTRA_ATTRIBUTES = { "import_prefix": attr.string(), diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockProtoSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockProtoSupport.java index f95336c507b283..bd3d56de0e12b1 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/MockProtoSupport.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockProtoSupport.java @@ -34,13 +34,19 @@ private MockProtoSupport() { public static void setup(MockToolsConfig config) throws IOException { createNetProto2(config); setupWorkspace(config); - config.append("tools/proto/toolchains/BUILD"); + registerProtoToolchain(config); } - /** - * Create a dummy "net/proto2 compiler and proto APIs for all languages - * and versions. - */ + private static void registerProtoToolchain(MockToolsConfig config) throws IOException { + config.append("WORKSPACE", "register_toolchains('tools/proto/toolchains:all')"); + config.append( + "tools/proto/toolchains/BUILD", + TestConstants.LOAD_PROTO_TOOLCHAIN, + "proto_toolchain(name = 'protoc_sources'," + + "proto_compiler = '//net/proto2/compiler/public:protocol_compiler')"); + } + + /** Create a dummy "net/proto2 compiler and proto APIs for all languages and versions. */ private static void createNetProto2(MockToolsConfig config) throws IOException { config.create( "net/proto2/compiler/public/BUILD", @@ -203,8 +209,12 @@ public static void setupWorkspace(MockToolsConfig config) throws IOException { " path = 'third_party/bazel_rules/rules_proto',", ")"); } + config.create("third_party/bazel_rules/rules_proto/WORKSPACE"); - config.create("third_party/bazel_rules/rules_proto/proto/BUILD", "licenses(['notice'])"); + config.create( + "third_party/bazel_rules/rules_proto/proto/BUILD", + "licenses(['notice'])", + "toolchain_type(name = 'toolchain_type', visibility = ['//visibility:public'])"); config.create( "third_party/bazel_rules/rules_proto/proto/defs.bzl", "def _add_tags(kargs):", @@ -216,5 +226,58 @@ public static void setupWorkspace(MockToolsConfig config) throws IOException { "", "def proto_library(**kargs): native.proto_library(**_add_tags(kargs))", "def proto_lang_toolchain(**kargs): native.proto_lang_toolchain(**_add_tags(kargs))"); + config.create( + "third_party/bazel_rules/rules_proto/proto/proto_toolchain.bzl", + "load(':proto_toolchain_rule.bzl', _proto_toolchain_rule = 'proto_toolchain')", + "def proto_toolchain(*, name, proto_compiler, exec_compatible_with = []):", + " _proto_toolchain_rule(name = name, proto_compiler = proto_compiler)", + " native.toolchain(", + " name = name + '_toolchain',", + " toolchain_type = '" + TestConstants.PROTO_TOOLCHAIN + "',", + " exec_compatible_with = exec_compatible_with,", + " target_compatible_with = [],", + " toolchain = name,", + " )"); + config.create( + "third_party/bazel_rules/rules_proto/proto/proto_toolchain_rule.bzl", + "ProtoLangToolchainInfo = proto_common_do_not_use.ProtoLangToolchainInfo", + "def _impl(ctx):", + " return [", + " DefaultInfo(", + " files = depset(),", + " runfiles = ctx.runfiles(),", + " ),", + " platform_common.ToolchainInfo(", + " proto = ProtoLangToolchainInfo(", + " out_replacement_format_flag = ctx.attr.command_line,", + " output_files = ctx.attr.output_files,", + " plugin = None,", + " runtime = None,", + " proto_compiler = ctx.attr.proto_compiler.files_to_run,", + " protoc_opts = ctx.fragments.proto.experimental_protoc_opts,", + " progress_message = ctx.attr.progress_message,", + " mnemonic = ctx.attr.mnemonic,", + " allowlist_different_package = None,", + " ),", + " ),", + " ]", + "proto_toolchain = rule(", + " _impl,", + " attrs = {", + " 'progress_message': attr.string(default = ", + " 'Generating Descriptor Set proto_library %{label}'),", + " 'mnemonic': attr.string(default = 'GenProtoDescriptorSet'),", + " 'command_line': attr.string(default = '--descriptor_set_out=%s'),", + " 'output_files': attr.string(values = ['single', 'multiple', 'legacy'],", + " default = 'single'),", + " 'proto_compiler': attr.label(", + " cfg = 'exec',", + " executable = True,", + " allow_files = True,", + " ),", + " },", + " provides = [platform_common.ToolchainInfo],", + " fragments = ['proto'],", + ")"); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/java/proto/StarlarkJavaLiteProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/java/proto/StarlarkJavaLiteProtoLibraryTest.java index 153d508a04b88c..11c351d7834a23 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/java/proto/StarlarkJavaLiteProtoLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/java/proto/StarlarkJavaLiteProtoLibraryTest.java @@ -64,6 +64,7 @@ public final void setUpMocks() throws Exception { scratch.file("proto/BUILD", "licenses(['notice'])", "exports_files(['compiler'])"); mockToolchains(); + invalidatePackages(); actionsTestUtil = actionsTestUtil(); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java index 161aa6bd2c943f..2a43a40207c50c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.packages.util.MockProtoSupport; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.List; import org.junit.Before; import org.junit.Ignore; @@ -54,6 +55,19 @@ public void setUp() throws Exception { invalidatePackages(); } + @Test + public void protoToolchainResolution_enabled() throws Exception { + setBuildLanguageOptions("--incompatible_enable_proto_toolchain_resolution"); + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "proto_library(name='foo', srcs=['foo.proto'])"); + + getDescriptorOutput("//x:foo"); + + assertNoEvents(); + } + @Test public void createsDescriptorSets() throws Exception { scratch.file( @@ -989,6 +1003,7 @@ public void testStripImportPrefixForExternalRepositories() throws Exception { assertThat(commandLine).contains("-I" + genfiles + "/external/foo/x/y/_virtual_imports/q"); } + @CanIgnoreReturnValue private Artifact getDescriptorOutput(String label) throws Exception { return getFirstArtifactEndingWith(getFilesToBuild(getConfiguredTarget(label)), ".proto.bin"); } diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java index 69b11d5e40b4ce..fb719021fb773c 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java @@ -26,6 +26,9 @@ public class TestConstants { public static final String LOAD_PROTO_LIBRARY = "load('@rules_proto//proto:defs.bzl', 'proto_library')"; + public static final String PROTO_TOOLCHAIN = "@rules_proto//proto:toolchain_type"; + public static final String LOAD_PROTO_TOOLCHAIN = + "load('@rules_proto//proto:proto_toolchain.bzl', 'proto_toolchain')"; public static final String LOAD_PROTO_LANG_TOOLCHAIN = "load('@rules_proto//proto:defs.bzl', 'proto_lang_toolchain')";