Skip to content

Commit

Permalink
Remove {direct,transitive}OriginalSources field from ProtoInfo
Browse files Browse the repository at this point in the history
The information is already part of another field.

Cleanup after bazelbuild#12431 and bazelbuild#10939
  • Loading branch information
Yannic committed Dec 17, 2020
1 parent a0a1eca commit 0739386
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 68 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ private static void checkProtoLibrariesInDeps(
private boolean areSrcsBlacklisted() {
return !new ProtoSourceFileBlacklist(
ruleContext, getProtoToolchainProvider().blacklistedProtos())
.checkSrcs(protoInfo.getOriginalDirectProtoSources(), "cc_proto_library");
.checkSrcs(protoInfo.getDirectSources(), "cc_proto_library");
}

private FeatureConfiguration getFeatureConfiguration() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ void addProviders(ConfiguredAspect.Builder aspect) throws InterruptedException {
* proto_library.
*/
private boolean shouldGenerateCode() {
if (protoInfo.getOriginalDirectProtoSources().isEmpty()) {
if (protoInfo.getDirectSources().isEmpty()) {
return false;
}

Expand All @@ -274,8 +274,7 @@ private boolean shouldGenerateCode() {

protoBlackList = new ProtoSourceFileBlacklist(ruleContext, blacklistedProtos.build());

return protoBlackList.checkSrcs(
protoInfo.getOriginalDirectProtoSources(), "java_proto_library");
return protoBlackList.checkSrcs(protoInfo.getDirectSources(), "java_proto_library");
}

private void createProtoCompileAction(Artifact sourceJar) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,19 +131,6 @@ private static NestedSet<Artifact> computeTransitiveProtoSourceArtifacts(
return result.build();
}

private static NestedSet<Artifact> computeTransitiveOriginalProtoSources(
ImmutableList<ProtoInfo> protoDeps, ImmutableList<Artifact> originalProtoSources) {
NestedSetBuilder<Artifact> result = NestedSetBuilder.naiveLinkOrder();

result.addAll(originalProtoSources);

for (ProtoInfo dep : protoDeps) {
result.addTransitive(dep.getOriginalTransitiveProtoSources());
}

return result.build();
}

static NestedSet<Artifact> computeDependenciesDescriptorSets(ImmutableList<ProtoInfo> deps) {
return computeTransitiveDescriptorSets(null, deps);
}
Expand Down Expand Up @@ -390,8 +377,6 @@ public static ProtoInfo createProtoInfo(
NestedSet<ProtoSource> transitiveSources = computeTransitiveProtoSources(deps, library);
NestedSet<Artifact> transitiveProtoSources =
computeTransitiveProtoSourceArtifacts(directSources, deps);
NestedSet<Artifact> transitiveOriginalProtoSources =
computeTransitiveOriginalProtoSources(deps, originalDirectProtoSources);
NestedSet<String> transitiveProtoSourceRoots =
computeTransitiveProtoSourceRoots(deps, directProtoSourceRoot.getSafePathString());
NestedSet<Artifact> strictImportableProtosForDependents =
Expand All @@ -413,7 +398,6 @@ public static ProtoInfo createProtoInfo(
directProtoSourceRoot,
transitiveSources,
transitiveProtoSources,
transitiveOriginalProtoSources,
transitiveProtoSourceRoots,
strictImportableProtosForDependents,
directDescriptorSet,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ private static ToolchainInvocation createDescriptorSetToolchain(
"--descriptor_set_out=$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ null,
/* blacklistedProtos= */ NestedSetBuilder.<Artifact>emptySet(STABLE_ORDER)),
/* providedProtoSources= */ ImmutableList.of()),
outReplacement,
protocOpts.build());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,22 +51,11 @@ private static ImmutableList<Artifact> extractProtoSources(ImmutableList<ProtoSo
return builder.build();
}

private static ImmutableList<Artifact> extractOriginalProtoSources(
ImmutableList<ProtoSource> sources) {
ImmutableList.Builder<Artifact> builder = ImmutableList.builder();
for (ProtoSource source : sources) {
builder.add(source.getOriginalSourceFile());
}
return builder.build();
}

private final ImmutableList<ProtoSource> directSources;
private final ImmutableList<Artifact> directProtoSources;
private final ImmutableList<Artifact> originalDirectProtoSources;
private final PathFragment directProtoSourceRoot;
private final NestedSet<ProtoSource> transitiveSources;
private final NestedSet<Artifact> transitiveProtoSources;
private final NestedSet<Artifact> originalTransitiveProtoSources;
private final NestedSet<String> transitiveProtoSourceRoots;
private final NestedSet<Artifact> strictImportableProtoSourcesForDependents;
private final Artifact directDescriptorSet;
Expand All @@ -85,7 +74,6 @@ public ProtoInfo(
PathFragment directProtoSourceRoot,
NestedSet<ProtoSource> transitiveSources,
NestedSet<Artifact> transitiveProtoSources,
NestedSet<Artifact> originalTransitiveProtoSources,
NestedSet<String> transitiveProtoSourceRoots,
NestedSet<Artifact> strictImportableProtoSourcesForDependents,
Artifact directDescriptorSet,
Expand All @@ -96,11 +84,9 @@ public ProtoInfo(
NestedSet<ProtoSource> 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;
Expand Down Expand Up @@ -130,16 +116,6 @@ public ImmutableList<Artifact> getDirectProtoSources() {
return directProtoSources;
}

/**
* The non-virtual proto sources of the {@code proto_library} declaring this provider.
*
* <p>Different from {@link #getDirectProtoSources()} if a transitive dependency has {@code
* import_prefix} or the like.
*/
public ImmutableList<Artifact> getOriginalDirectProtoSources() {
return originalDirectProtoSources;
}

/**
* The source root of the current library.
*
Expand Down Expand Up @@ -169,16 +145,6 @@ public NestedSet<Artifact> getTransitiveProtoSources() {
return transitiveProtoSources;
}

/**
* The non-virtual transitive proto source files.
*
* <p>Different from {@link #getTransitiveProtoSources()} if a transitive dependency has {@code
* import_prefix} or the like.
*/
public NestedSet<Artifact> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ public class ProtoLangToolchain implements RuleConfiguredTargetFactory {
@Override
public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
NestedSetBuilder<Artifact> blacklistedProtos = NestedSetBuilder.stableOrder();
NestedSetBuilder<ProtoSource> providedProtoSources = NestedSetBuilder.stableOrder();
for (ProtoInfo protoInfo :
ruleContext.getPrerequisites("blacklisted_protos", ProtoInfo.PROVIDER)) {
blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources());
providedProtoSources.addTransitive(protoInfo.getTransitiveSources());
}

return new RuleConfiguredTargetBuilder(ruleContext)
Expand All @@ -45,7 +45,14 @@ public ConfiguredTarget create(RuleContext ruleContext)
ruleContext.attributes().get("command_line", Type.STRING),
ruleContext.getPrerequisite("plugin", FilesToRunProvider.class),
ruleContext.getPrerequisite("runtime"),
blacklistedProtos.build()))
// We intentionally flatten the NestedSet here.
//
// `providedProtoSources` are read during analysis, so flattening the set here once
// avoid flattening it in every `<lang>_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.<Artifact>emptySet(STABLE_ORDER))
.addProvider(RunfilesProvider.simple(Runfiles.EMPTY))
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -40,15 +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 <lang>_proto_library} should not generate bindings.
*/
public abstract ImmutableList<ProtoSource> providedProtoSources();

@Deprecated
public abstract NestedSet<Artifact> blacklistedProtos();

@AutoCodec.Instantiator
public static ProtoLangToolchainProvider createForDeserialization(
String commandLine,
FilesToRunProvider pluginExecutable,
TransitiveInfoCollection runtime,
ImmutableList<ProtoSource> providedProtoSources,
NestedSet<Artifact> blacklistedProtos) {
return new AutoValue_ProtoLangToolchainProvider(
commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos);
}

public static ProtoLangToolchainProvider create(
String commandLine,
FilesToRunProvider pluginExecutable,
TransitiveInfoCollection runtime,
ImmutableList<ProtoSource> providedProtoSources) {
NestedSetBuilder<Artifact> blacklistedProtos = NestedSetBuilder.stableOrder();
for (ProtoSource protoSource : providedProtoSources) {
blacklistedProtos.add(protoSource.getOriginalSourceFile());
}
return new AutoValue_ProtoLangToolchainProvider(
commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos.build());
}

@Deprecated
public static ProtoLangToolchainProvider create(
String commandLine,
FilesToRunProvider pluginExecutable,
TransitiveInfoCollection runtime,
NestedSet<Artifact> blacklistedProtos) {
return new AutoValue_ProtoLangToolchainProvider(
commandLine, pluginExecutable, runtime, blacklistedProtos);
commandLine, pluginExecutable, runtime, ImmutableList.of(), blacklistedProtos);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -83,10 +84,19 @@ public Iterable<Artifact> filter(Iterable<Artifact> protoFiles) {
* Checks the proto sources for mixing blacklisted and non-blacklisted 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<ProtoSource> protoSources, String topLevelProtoRuleName) {
ImmutableList.Builder<Artifact> protoFiles = ImmutableList.builder();
for (ProtoSource protoSource : protoSources) {
protoFiles.add(protoSource.getOriginalSourceFile());
}
return checkSrcs(protoFiles.build(), topLevelProtoRuleName);
}

@Deprecated
public boolean checkSrcs(Iterable<Artifact> protoFiles, String topLevelProtoRuleName) {
List<Artifact> blacklisted = new ArrayList<>();
List<Artifact> nonBlacklisted = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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),
Expand All @@ -105,14 +104,14 @@ public void commandLine_basic() throws Exception {
"--java_out=param1,param2:$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

ProtoLangToolchainProvider toolchainWithPlugin =
ProtoLangToolchainProvider.create(
"--$(PLUGIN_OUT)=param3,param4:$(OUT)",
plugin,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

CustomCommandLine cmdLine =
createCommandLineFromToolchains(
Expand Down Expand Up @@ -175,7 +174,7 @@ public void commandLine_strictDeps() throws Exception {
"--java_out=param1,param2:$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

CustomCommandLine cmdLine =
createCommandLineFromToolchains(
Expand Down Expand Up @@ -213,7 +212,7 @@ public void commandLine_exports() throws Exception {
"--java_out=param1,param2:$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

CustomCommandLine cmdLine =
createCommandLineFromToolchains(
Expand Down Expand Up @@ -283,7 +282,7 @@ public String toString() {
"--java_out=param1,param2:$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

CustomCommandLine cmdLine =
createCommandLineFromToolchains(
Expand Down Expand Up @@ -317,14 +316,14 @@ public void exceptionIfSameName() throws Exception {
"dontcare",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

ProtoLangToolchainProvider toolchain2 =
ProtoLangToolchainProvider.create(
"dontcare",
/* pluginExecutable= */ null,
/* runtime= */ mock(TransitiveInfoCollection.class),
/* blacklistedProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER));
/* providedProtoSources= */ ImmutableList.of());

IllegalStateException e =
assertThrows(
Expand Down

0 comments on commit 0739386

Please sign in to comment.