Create experimental flag to include source info in proto descriptors `protoc` has an option to include source info (e.g. comments) in the generated descriptor sets. Well done proto generators also relay the comments into the generated source so IDEs that do source indexing and pick them up to provide contextual tooltips/documentation/etc. This change adds an experimental flag to include this info in the descriptors generated by `proto_library` so we can collect some data whether including source info by default is feasible. See #9337 for more context. //cc @thomasvl Closes #10925. PiperOrigin-RevId: 299838533
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java index f26499a..1694a3f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
@@ -14,12 +14,12 @@ package com.google.devtools.build.lib.rules.proto; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Iterables.isEmpty; import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER; import static com.google.devtools.build.lib.rules.proto.ProtoCommon.areDepsStrict; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -337,7 +337,9 @@ SpawnAction.Builder actions = createActions( ruleContext, - ImmutableList.of(createDescriptorSetToolchain(output.getExecPathString())), + ImmutableList.of( + createDescriptorSetToolchain( + ruleContext.getFragment(ProtoConfiguration.class), output.getExecPathString())), protoInfo, ruleContext.getLabel(), ImmutableList.of(output), @@ -353,7 +355,13 @@ ruleContext.registerAction(actions.build(ruleContext)); } - private static ToolchainInvocation createDescriptorSetToolchain(CharSequence outReplacement) { + private static ToolchainInvocation createDescriptorSetToolchain( + ProtoConfiguration configuration, CharSequence outReplacement) { + ImmutableList.Builder<String> protocOpts = ImmutableList.builder(); + if (configuration.experimentalProtoDescriptorSetsIncludeSourceInfo()) { + protocOpts.add("--include_source_info"); + } + return new ToolchainInvocation( "dontcare", ProtoLangToolchainProvider.create( @@ -365,7 +373,8 @@ /* pluginExecutable= */ null, /* runtime= */ null, /* blacklistedProtos= */ NestedSetBuilder.<Artifact>emptySet(STABLE_ORDER)), - outReplacement); + outReplacement, + protocOpts.build()); } /** Whether to use exports in the proto compile action. */ @@ -552,6 +561,8 @@ "--plugin=protoc-gen-PLUGIN_%s=%s", invocation.name, toolchain.pluginExecutable().getExecutable().getExecPath()); } + + cmdLine.addAll(invocation.protocOpts); } cmdLine.addAll(protocOpts); @@ -749,13 +760,23 @@ final String name; public final ProtoLangToolchainProvider toolchain; final CharSequence outReplacement; + final ImmutableList<String> protocOpts; public ToolchainInvocation( String name, ProtoLangToolchainProvider toolchain, CharSequence outReplacement) { - checkState(!name.contains(" "), "Name %s should not contain spaces", name); + this(name, toolchain, outReplacement, ImmutableList.of()); + } + + public ToolchainInvocation( + String name, + ProtoLangToolchainProvider toolchain, + CharSequence outReplacement, + ImmutableList<String> protocOpts) { + Preconditions.checkState(!name.contains(" "), "Name %s should not contain spaces", name); this.name = name; this.toolchain = toolchain; this.outReplacement = outReplacement; + this.protocOpts = Preconditions.checkNotNull(protocOpts); } } }
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 9ada5aa..dfe5e9e 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
@@ -75,6 +75,15 @@ public boolean experimentalProtoExtraActions; @Option( + name = "experimental_proto_descriptor_sets_include_source_info", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + help = "Run extra actions for alternative Java api versions in a proto_library.") + public boolean experimentalProtoDescriptorSetsIncludeSourceInfo; + + @Option( name = "proto_compiler", defaultValue = "@com_google_protobuf//:protoc", converter = CoreOptionConverters.LabelConverter.class, @@ -181,6 +190,8 @@ host.loadProtoRulesFromBzl = loadProtoRulesFromBzl; host.protoCompiler = protoCompiler; host.protocOpts = protocOpts; + host.experimentalProtoDescriptorSetsIncludeSourceInfo = + experimentalProtoDescriptorSetsIncludeSourceInfo; host.experimentalProtoExtraActions = experimentalProtoExtraActions; host.protoCompiler = protoCompiler; host.protoToolchainForJava = protoToolchainForJava; @@ -234,6 +245,10 @@ return protocOpts; } + public boolean experimentalProtoDescriptorSetsIncludeSourceInfo() { + return options.experimentalProtoDescriptorSetsIncludeSourceInfo; + } + /** * Returns true if we will run extra actions for actions that are not run by default. If this * is enabled, e.g. all extra_actions for alternative api-versions or language-flavours of a
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 a905862..d3d2608 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
@@ -1003,4 +1003,41 @@ getConfiguredTarget("//a"); } + + @Test + public void testNoExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Exception { + if (!isThisBazel()) { + return; + } + + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "proto_library(", + " name = 'a_proto',", + " srcs = ['a.proto'],", + ")"); + + Iterable<String> commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto")); + assertThat(commandLine).doesNotContain("--include_source_info"); + } + + @Test + public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Exception { + if (!isThisBazel()) { + return; + } + + useConfiguration("--experimental_proto_descriptor_sets_include_source_info"); + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "proto_library(", + " name = 'a_proto',", + " srcs = ['a.proto'],", + ")"); + + Iterable<String> commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto")); + assertThat(commandLine).contains("--include_source_info"); + } }