From 3016af5905341f814a9cea74837d4376a0dcfc49 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Sun, 8 Mar 2020 19:29:34 +0100 Subject: [PATCH] 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 information whether including source info by default is feasible. See #9337 for more context. --- .../proto/ProtoCompileActionBuilder.java | 32 ++++++++++++++++--- .../lib/rules/proto/ProtoConfiguration.java | 16 ++++++++++ .../rules/proto/BazelProtoLibraryTest.java | 29 +++++++++++++++++ 3 files changed, 72 insertions(+), 5 deletions(-) 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 f26499aaf8b4e1..709f4980206211 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,10 @@ public static void writeDescriptorSet( 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 +356,13 @@ public static void writeDescriptorSet( ruleContext.registerAction(actions.build(ruleContext)); } - private static ToolchainInvocation createDescriptorSetToolchain(CharSequence outReplacement) { + private static ToolchainInvocation createDescriptorSetToolchain( + ProtoConfiguration configuration, CharSequence outReplacement) { + ImmutableList.Builder protocOpts = ImmutableList.builder(); + if (configuration.experimentalProtoDescriptorSetsIncludeSourceInfo()) { + protocOpts.add("--include_source_info"); + } + return new ToolchainInvocation( "dontcare", ProtoLangToolchainProvider.create( @@ -365,7 +374,8 @@ private static ToolchainInvocation createDescriptorSetToolchain(CharSequence out /* pluginExecutable= */ null, /* runtime= */ null, /* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)), - outReplacement); + outReplacement, + protocOpts.build()); } /** Whether to use exports in the proto compile action. */ @@ -552,6 +562,8 @@ static CustomCommandLine createCommandLineFromToolchains( "--plugin=protoc-gen-PLUGIN_%s=%s", invocation.name, toolchain.pluginExecutable().getExecutable().getExecPath()); } + + cmdLine.addAll(invocation.protocOpts); } cmdLine.addAll(protocOpts); @@ -749,13 +761,23 @@ public static class ToolchainInvocation { final String name; public final ProtoLangToolchainProvider toolchain; final CharSequence outReplacement; + final ImmutableList 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 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 ba8725eb45bf87..d56e2bba59022e 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 @@ -81,6 +81,16 @@ public static class Options extends FragmentOptions { ) 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", @@ -208,6 +218,8 @@ public FragmentOptions getHost() { loadProtoToolchainForJavaliteFromComGoogleProtobuf; host.protoCompiler = protoCompiler; host.protocOpts = protocOpts; + host.experimentalProtoDescriptorSetsIncludeSourceInfo = + experimentalProtoDescriptorSetsIncludeSourceInfo; host.experimentalProtoExtraActions = experimentalProtoExtraActions; host.protoCompiler = protoCompiler; host.protoToolchainForJava = protoToolchainForJava; @@ -261,6 +273,10 @@ public ImmutableList protocOpts() { 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 a90586273e7e83..17843d79487c5d 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,33 @@ public void testMigrationLabelNotRequiredWhenDisabled() throws Exception { getConfiguredTarget("//a"); } + + @Test + public void testNoExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Exception { + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "proto_library(", + " name = 'a_proto',", + " srcs = ['a.proto'],", + ")"); + + Iterable commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto")); + assertThat(commandLine).doesNotContain("--include_source_info"); + } + + @Test + public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Exception { + 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 commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto")); + assertThat(commandLine).contains("--include_source_info"); + } }