diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java index 051cc2f168fb22..689d29f2623087 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java @@ -285,7 +285,7 @@ private static void checkProtoLibrariesInDeps( private boolean areSrcsExcluded() { return !new ProtoSourceFileExcludeList( ruleContext, getProtoToolchainProvider().forbiddenProtos()) - .checkSrcs(protoInfo.getOriginalDirectProtoSources(), "cc_proto_library"); + .checkSrcs(protoInfo.getDirectSources(), "cc_proto_library"); } private FeatureConfiguration getFeatureConfiguration() throws RuleErrorException { diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java index f794dbd1ffc105..7beac8ece1361f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java @@ -273,7 +273,7 @@ void addProviders(ConfiguredAspect.Builder aspect) throws InterruptedException { * proto_library. */ private boolean shouldGenerateCode() { - if (protoInfo.getOriginalDirectProtoSources().isEmpty()) { + if (protoInfo.getDirectSources().isEmpty()) { return false; } @@ -285,7 +285,7 @@ private boolean shouldGenerateCode() { new ProtoSourceFileExcludeList(ruleContext, forbiddenProtos.build()); return protoExcludeList.checkSrcs( - protoInfo.getOriginalDirectProtoSources(), "java_proto_library"); + protoInfo.getDirectSources(), "java_proto_library"); } private void createProtoCompileAction(Artifact sourceJar) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java index e7f8f1d11626ec..5fc6f9d1c4c140 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java @@ -131,19 +131,6 @@ private static NestedSet computeTransitiveProtoSourceArtifacts( return result.build(); } - private static NestedSet computeTransitiveOriginalProtoSources( - ImmutableList protoDeps, ImmutableList originalProtoSources) { - NestedSetBuilder result = NestedSetBuilder.naiveLinkOrder(); - - result.addAll(originalProtoSources); - - for (ProtoInfo dep : protoDeps) { - result.addTransitive(dep.getOriginalTransitiveProtoSources()); - } - - return result.build(); - } - static NestedSet computeDependenciesDescriptorSets(ImmutableList deps) { return computeTransitiveDescriptorSets(null, deps); } @@ -390,8 +377,6 @@ public static ProtoInfo createProtoInfo( NestedSet transitiveSources = computeTransitiveProtoSources(deps, library); NestedSet transitiveProtoSources = computeTransitiveProtoSourceArtifacts(directSources, deps); - NestedSet transitiveOriginalProtoSources = - computeTransitiveOriginalProtoSources(deps, originalDirectProtoSources); NestedSet transitiveProtoSourceRoots = computeTransitiveProtoSourceRoots(deps, directProtoSourceRoot.getSafePathString()); NestedSet strictImportableProtosForDependents = @@ -413,7 +398,6 @@ public static ProtoInfo createProtoInfo( directProtoSourceRoot, transitiveSources, transitiveProtoSources, - transitiveOriginalProtoSources, transitiveProtoSourceRoots, strictImportableProtosForDependents, directDescriptorSet, 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 a13c9212500b9f..1be24ddfb79ed3 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 @@ -364,7 +364,7 @@ private static ToolchainInvocation createDescriptorSetToolchain( "--descriptor_set_out=$(OUT)", /* pluginExecutable= */ null, /* runtime= */ null, - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)), + /* providedProtoSources= */ ImmutableList.of()), outReplacement, protocOpts.build()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java index 93197a262441d4..15a44346c8aca9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java @@ -51,22 +51,11 @@ private static ImmutableList extractProtoSources(ImmutableList extractOriginalProtoSources( - ImmutableList sources) { - ImmutableList.Builder builder = ImmutableList.builder(); - for (ProtoSource source : sources) { - builder.add(source.getOriginalSourceFile()); - } - return builder.build(); - } - private final ImmutableList directSources; private final ImmutableList directProtoSources; - private final ImmutableList originalDirectProtoSources; private final PathFragment directProtoSourceRoot; private final NestedSet transitiveSources; private final NestedSet transitiveProtoSources; - private final NestedSet originalTransitiveProtoSources; private final NestedSet transitiveProtoSourceRoots; private final NestedSet strictImportableProtoSourcesForDependents; private final Artifact directDescriptorSet; @@ -85,7 +74,6 @@ public ProtoInfo( PathFragment directProtoSourceRoot, NestedSet transitiveSources, NestedSet transitiveProtoSources, - NestedSet originalTransitiveProtoSources, NestedSet transitiveProtoSourceRoots, NestedSet strictImportableProtoSourcesForDependents, Artifact directDescriptorSet, @@ -96,11 +84,9 @@ public ProtoInfo( NestedSet publicImportSources) { this.directSources = directSources; this.directProtoSources = extractProtoSources(directSources); - this.originalDirectProtoSources = extractOriginalProtoSources(directSources); this.directProtoSourceRoot = ProtoCommon.memoryEfficientProtoSourceRoot(directProtoSourceRoot); this.transitiveSources = transitiveSources; this.transitiveProtoSources = transitiveProtoSources; - this.originalTransitiveProtoSources = originalTransitiveProtoSources; this.transitiveProtoSourceRoots = transitiveProtoSourceRoots; this.strictImportableProtoSourcesForDependents = strictImportableProtoSourcesForDependents; this.directDescriptorSet = directDescriptorSet; @@ -130,16 +116,6 @@ public ImmutableList getDirectProtoSources() { return directProtoSources; } - /** - * The non-virtual proto sources of the {@code proto_library} declaring this provider. - * - *

Different from {@link #getDirectProtoSources()} if a transitive dependency has {@code - * import_prefix} or the like. - */ - public ImmutableList getOriginalDirectProtoSources() { - return originalDirectProtoSources; - } - /** * The source root of the current library. * @@ -169,16 +145,6 @@ public NestedSet getTransitiveProtoSources() { return transitiveProtoSources; } - /** - * The non-virtual transitive proto source files. - * - *

Different from {@link #getTransitiveProtoSources()} if a transitive dependency has {@code - * import_prefix} or the like. - */ - public NestedSet getOriginalTransitiveProtoSources() { - return originalTransitiveProtoSources; - } - /** * The proto source roots of the transitive closure of this rule. These flags will be passed to * {@code protoc} in the specified order, via the {@code --proto_path} flag. diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java index 96dda1fce6a3ed..fbc331d35be35d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java @@ -33,10 +33,10 @@ public class ProtoLangToolchain implements RuleConfiguredTargetFactory { @Override public ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException, RuleErrorException, ActionConflictException { - NestedSetBuilder forbiddenProtos = NestedSetBuilder.stableOrder(); + NestedSetBuilder providedProtoSources = NestedSetBuilder.stableOrder(); for (ProtoInfo protoInfo : ruleContext.getPrerequisites("blacklisted_protos", ProtoInfo.PROVIDER)) { - forbiddenProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources()); + providedProtoSources.addTransitive(protoInfo.getTransitiveSources()); } return new RuleConfiguredTargetBuilder(ruleContext) @@ -45,7 +45,14 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext.attributes().get("command_line", Type.STRING), ruleContext.getPrerequisite("plugin", FilesToRunProvider.class), ruleContext.getPrerequisite("runtime"), - forbiddenProtos.build())) + // We intentionally flatten the NestedSet here. + // + // `providedProtoSources` are read during analysis, so flattening the set here once + // avoid flattening it in every `_proto_aspect` applied to `proto_library` + // targets. While this has the potential to use more memory than using a NestedSet, + // there are only a few `proto_lang_toolchain` targets in every build, so the impact + // on memory consumption should be neglectable. + providedProtoSources.build().toList())) .setFilesToBuild(NestedSetBuilder.emptySet(STABLE_ORDER)) .addProvider(RunfilesProvider.simple(Runfiles.EMPTY)) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java index 23c310c11cf572..0a6bd07462e738 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java @@ -15,11 +15,13 @@ package com.google.devtools.build.lib.rules.proto; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import javax.annotation.Nullable; @@ -40,23 +42,46 @@ public abstract class ProtoLangToolchainProvider implements TransitiveInfoProvid @Nullable public abstract TransitiveInfoCollection runtime(); + /** + * Returns a list of {@link ProtoSource}s that are already provided by the protobuf runtime (i.e. + * for which {@code _proto_library} should not generate bindings. + */ + public abstract ImmutableList providedProtoSources(); + /** * This makes the blacklisted_protos member available in the provider. It can be removed after * users are migrated and a sufficient time for Bazel rules to migrate has elapsed. */ + @Deprecated public NestedSet blacklistedProtos() { return forbiddenProtos(); } + // TODO(yannic): Remove after migrating all users to `providedProtoSources()`. + @Deprecated public abstract NestedSet forbiddenProtos(); @AutoCodec.Instantiator + public static ProtoLangToolchainProvider createForDeserialization( + String commandLine, + FilesToRunProvider pluginExecutable, + TransitiveInfoCollection runtime, + ImmutableList providedProtoSources, + NestedSet blacklistedProtos) { + return new AutoValue_ProtoLangToolchainProvider( + commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos); + } + public static ProtoLangToolchainProvider create( String commandLine, FilesToRunProvider pluginExecutable, TransitiveInfoCollection runtime, - NestedSet forbiddenProtos) { + ImmutableList providedProtoSources) { + NestedSetBuilder blacklistedProtos = NestedSetBuilder.stableOrder(); + for (ProtoSource protoSource : providedProtoSources) { + blacklistedProtos.add(protoSource.getOriginalSourceFile()); + } return new AutoValue_ProtoLangToolchainProvider( - commandLine, pluginExecutable, runtime, forbiddenProtos); + commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos.build()); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java index cdccdcc69e0bcb..98d006450fd0e9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java @@ -20,6 +20,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Streams; import com.google.devtools.build.lib.actions.Artifact; @@ -80,10 +81,19 @@ public Iterable filter(Iterable protoFiles) { * Checks the proto sources for mixing excluded and non-excluded protos in one single * proto_library rule. Registers an attribute error if proto mixing is detected. * - * @param protoFiles the protos to filter. + * @param protoSources the protos to filter. * @param topLevelProtoRuleName the name of the top-level rule that generates the protos. * @return whether the proto sources are clean without mixing. */ + public boolean checkSrcs(ImmutableList protoSources, String topLevelProtoRuleName) { + ImmutableList.Builder protoFiles = ImmutableList.builder(); + for (ProtoSource protoSource : protoSources) { + protoFiles.add(protoSource.getOriginalSourceFile()); + } + return checkSrcs(protoFiles.build(), topLevelProtoRuleName); + } + + @Deprecated public boolean checkSrcs(Iterable protoFiles, String topLevelProtoRuleName) { List excluded = new ArrayList<>(); List nonExcluded = new ArrayList<>(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java index 2714d3a83bb44d..984d420666579e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java @@ -81,7 +81,6 @@ private ProtoInfo protoInfo( /* directProtoSourceRoot */ PathFragment.EMPTY_FRAGMENT, /* transitiveSources */ NestedSetBuilder.wrap(Order.STABLE_ORDER, transitiveProtoSources), /* transitiveProtoSources */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - /* originalTransitiveProtoSources */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /* transitiveProtoSourceRoots */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /* strictImportableProtoSourcesForDependents */ NestedSetBuilder.emptySet( Order.STABLE_ORDER), @@ -107,14 +106,14 @@ public void commandLine_basic() throws Exception { "--java_out=param1,param2:$(OUT)", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); ProtoLangToolchainProvider toolchainWithPlugin = ProtoLangToolchainProvider.create( "--$(PLUGIN_OUT)=param3,param4:$(OUT)", plugin, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); CustomCommandLine cmdLine = createCommandLineFromToolchains( @@ -177,7 +176,7 @@ public void commandLine_strictDeps() throws Exception { "--java_out=param1,param2:$(OUT)", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); CustomCommandLine cmdLine = createCommandLineFromToolchains( @@ -215,7 +214,7 @@ public void commandLine_exports() throws Exception { "--java_out=param1,param2:$(OUT)", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); CustomCommandLine cmdLine = createCommandLineFromToolchains( @@ -285,7 +284,7 @@ public String toString() { "--java_out=param1,param2:$(OUT)", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); CustomCommandLine cmdLine = createCommandLineFromToolchains( @@ -319,14 +318,14 @@ public void exceptionIfSameName() throws Exception { "dontcare", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); ProtoLangToolchainProvider toolchain2 = ProtoLangToolchainProvider.create( "dontcare", /* pluginExecutable= */ null, /* runtime= */ mock(TransitiveInfoCollection.class), - /* forbiddenProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER)); + /* providedProtoSources= */ ImmutableList.of()); IllegalStateException e = assertThrows(