Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove {direct,transitive}OriginalSources field from ProtoInfo #12727

Closed
wants to merge 1 commit into from
Closed

Remove {direct,transitive}OriginalSources field from ProtoInfo #12727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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) {
Expand Down
16 changes: 0 additions & 16 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ private static ToolchainInvocation createDescriptorSetToolchain(
"--descriptor_set_out=$(OUT)",
/* pluginExecutable= */ null,
/* runtime= */ null,
/* forbiddenProtos= */ NestedSetBuilder.<Artifact>emptySet(STABLE_ORDER)),
/* providedProtoSources= */ ImmutableList.of()),
outReplacement,
protocOpts.build());
}
Expand Down
34 changes: 0 additions & 34 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java
View file
Open in desktop
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
13 changes: 10 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java
View file
Open in desktop
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> forbiddenProtos = NestedSetBuilder.stableOrder();
NestedSetBuilder<ProtoSource> providedProtoSources = NestedSetBuilder.stableOrder();
for (ProtoInfo protoInfo :
ruleContext.getPrerequisites("blacklisted_protos", ProtoInfo.PROVIDER)) {
forbiddenProtos.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"),
forbiddenProtos.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()))
Comment on lines +48 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to flatten the NestedSet? If I understand this correctly, so that you can use overloaded ProtoLangToolchainProvider.create? I'd prefer if you remove the old implementation of create and just use NestedSet. I checked that there are no dependencies internally. If you're worried about other forks of Bazel, perhaps you could change the create method name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this not for overloading ProtoLangToolchainProvider#create(). Today, every <lang>_proto_library has to flatten the set to figure out which of its srcs is in blacklistedProtos(), which does have performance overhead. We can avoid that by flattening it here once and propagate the excluded protos as list instead. We trade some (finite) memory for performance.

.setFilesToBuild(NestedSetBuilder.<Artifact>emptySet(STABLE_ORDER))
.addProvider(RunfilesProvider.simple(Runfiles.EMPTY))
.build();
Expand Down
29 changes: 27 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainProvider.java
View file
Open in desktop
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,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 <lang>_proto_library} should not generate bindings.
*/
public abstract ImmutableList<ProtoSource> 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<Artifact> blacklistedProtos() {
return forbiddenProtos();
}

// TODO(yannic): Remove after migrating all users to `providedProtoSources()`.
@Deprecated
public abstract NestedSet<Artifact> forbiddenProtos();

@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,
NestedSet<Artifact> forbiddenProtos) {
ImmutableList<ProtoSource> providedProtoSources) {
NestedSetBuilder<Artifact> 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());
}
}
12 changes: 11 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourceFileExcludeList.java
View file
Open in desktop
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 @@ -80,10 +81,19 @@ public Iterable<Artifact> filter(Iterable<Artifact> 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<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> excluded = new ArrayList<>();
List<Artifact> nonExcluded = new ArrayList<>();
Expand Down
15 changes: 7 additions & 8 deletions src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down