Skip to content

Commit

Permalink
Remove import-path guessing from ProtoCompileActionBuilder
Browse files Browse the repository at this point in the history 
Instead of trying to guess the correct import-path of a proto file,
we save their associated source root in ProtoInfo and compute the
import path by taking the proto files's exec path relative to that
source root.
  • Loading branch information
@Yannic
Yannic committed Mar 18, 2020
1 parent be4b2dc commit 7ab5bd3
Show file tree
Hide file tree
Showing 5 changed files with 314 additions and 347 deletions.
119 changes: 58 additions & 61 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java
View file
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.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand Down Expand Up @@ -179,21 +180,18 @@ private static NestedSet<String> computeTransitiveProtoSourceRoots(
// TODO(lberki): Would be nice if had these in ProtoInfo instead of that haphazard set of fields
// Unfortunately, ProtoInfo has a Starlark interface so that requires a migration.
static final class Library {
private final ImmutableList<Artifact> sources;
private final ImmutableList<Pair<Artifact, String>> importPathSourcePair;
private final String sourceRoot;

Library(
ImmutableList<Artifact> sources,
String sourceRoot,
ImmutableList<Pair<Artifact, String>> importPathSourcePair) {
this.sources = sources;
this.sourceRoot = sourceRoot;
this.importPathSourcePair = importPathSourcePair;
}

public ImmutableList<Artifact> getSources() {
return sources;
return ImmutableList.copyOf(Iterables.transform(importPathSourcePair, p -> p.first));
}

public ImmutableList<Pair<Artifact, String>> getImportPathSourcePair() {
Expand Down Expand Up @@ -232,8 +230,7 @@ private static Library createLibraryWithoutVirtualSourceRoot(
for (Artifact protoSource : directSources) {
builder.add(new Pair<Artifact, String>(protoSource, null));
}
return new Library(
directSources, protoSourceRoot.isEmpty() ? "." : protoSourceRoot, builder.build());
return new Library(protoSourceRoot.isEmpty() ? "." : protoSourceRoot, builder.build());
}

private static PathFragment getPathFragmentAttribute(
Expand Down Expand Up @@ -335,7 +332,6 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
importPrefix = PathFragment.EMPTY_FRAGMENT;
}

ImmutableList.Builder<Artifact> symlinks = ImmutableList.builder();
ImmutableList.Builder<Pair<Artifact, String>> protoSourceImportPair = ImmutableList.builder();

PathFragment sourceRootPath = ruleContext.getUniqueDirectory("_virtual_imports");
Expand All @@ -358,8 +354,7 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
importPrefix,
stripImportPrefix,
starlarkSemantics.experimentalSiblingRepositoryLayout());
protoSourceImportPair.add(new Pair<>(realProtoSource, importsPair.first.toString()));
symlinks.add(importsPair.second);
protoSourceImportPair.add(new Pair<>(importsPair.second, importsPair.first.toString()));
}

String sourceRoot =
Expand All @@ -368,7 +363,7 @@ private static Library createLibraryWithVirtualSourceRootMaybe(
.getExecPath()
.getRelative(sourceRootPath)
.getPathString();
return new Library(symlinks.build(), sourceRoot, protoSourceImportPair.build());
return new Library(sourceRoot, protoSourceImportPair.build());
}

private static Pair<PathFragment, Artifact> computeImports(
Expand Down Expand Up @@ -404,44 +399,57 @@ private static Pair<PathFragment, Artifact> computeImports(
}

/**
* Returns a set of the {@code proto_source_root} collected from the current library and the
* specified attribute.
*
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
* Returns a set of pairs of {@code proto_source_root -> [sources]} of `.proto` files that are
* exported by a {@code proto_library} target.
*/
private static NestedSet<String> getProtoSourceRootsOfAttribute(
RuleContext ruleContext, String currentProtoSourceRoot, String attributeName) {
NestedSetBuilder<String> protoSourceRoots = NestedSetBuilder.stableOrder();
protoSourceRoots.add(currentProtoSourceRoot);

for (ProtoInfo provider :
ruleContext.getPrerequisites(attributeName, Mode.TARGET, ProtoInfo.PROVIDER)) {
protoSourceRoots.add(provider.getDirectProtoSourceRoot());
private static NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> computeImportableProtos(
RuleContext ruleContext, Library library) {
NestedSetBuilder<Pair<PathFragment, ImmutableList<Artifact>>> protos =
NestedSetBuilder.stableOrder();
protos.add(new Pair<>(PathFragment.create(library.getSourceRoot()), library.getSources()));
for (ProtoInfo info : ruleContext.getPrerequisites("deps", TARGET, ProtoInfo.PROVIDER)) {
protos.addTransitive(info.getExportedProtos());
}

return protoSourceRoots.build();
return protos.build();
}

/**
* Returns a set of the {@code proto_source_root} collected from the current library and the
* direct dependencies.
*
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
* Returns a set of pairs of {@code proto_source_root -> [sources]} of `.proto` files that are
* exported by a {@code proto_library} target.
*/
private static NestedSet<String> computeStrictImportableProtoSourceRoots(
RuleContext ruleContext, String currentProtoSourceRoot) {
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "deps");
private static NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> computeExportedProtos(
RuleContext ruleContext, Library library) {
NestedSetBuilder<Pair<PathFragment, ImmutableList<Artifact>>> protos =
NestedSetBuilder.stableOrder();
protos.add(new Pair<>(PathFragment.create(library.getSourceRoot()), library.getSources()));
for (ProtoInfo info : ruleContext.getPrerequisites("exports", TARGET, ProtoInfo.PROVIDER)) {
protos.addTransitive(info.getExportedProtos());
}
if (library.getSources().isEmpty()) {
// This is an alias library (i.e. a `proto_library` target without `srcs`). Treat all exports
// as deps.
// TODO(yannic): Consider requiring users to use `exports` in this case.
for (ProtoInfo info : ruleContext.getPrerequisites("deps", TARGET, ProtoInfo.PROVIDER)) {
protos.addTransitive(info.getExportedProtos());
}
}
return protos.build();
}

/**
* Returns a set of the {@code proto_source_root} collected from the current library and the
* exported dependencies.
*
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
* Returns a set of pairs of {@code proto_source_root -> [sources]} of `.proto` files that are
* reachable by a {@code proto_library} target.
*/
private static NestedSet<String> computeExportedProtoSourceRoots(
RuleContext ruleContext, String currentProtoSourceRoot) {
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "exports");
private static NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> computeTransitiveProtos(
RuleContext ruleContext,
NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> exportedProtos) {
NestedSetBuilder<Pair<PathFragment, ImmutableList<Artifact>>> protos =
NestedSetBuilder.stableOrder();
protos.addTransitive(exportedProtos);
for (ProtoInfo info : ruleContext.getPrerequisites("deps", TARGET, ProtoInfo.PROVIDER)) {
protos.addTransitive(info.getTransitiveProtos());
}
return protos.build();
}

/**
Expand Down Expand Up @@ -521,13 +529,6 @@ public static ProtoInfo createProtoInfo(
ruleContext, library.getImportPathSourcePair());
NestedSet<Pair<Artifact, String>> strictImportableProtos =
computeStrictImportableProtos(ruleContext, library.getImportPathSourcePair());
NestedSet<String> strictImportableProtoSourceRoots =
computeStrictImportableProtoSourceRoots(ruleContext, library.getSourceRoot());

NestedSet<Pair<Artifact, String>> exportedProtos =
ProtoCommon.computeExportedProtos(ruleContext);
NestedSet<String> exportedProtoSourceRoots =
computeExportedProtoSourceRoots(ruleContext, library.getSourceRoot());

Artifact directDescriptorSet =
ruleContext.getGenfilesArtifact(
Expand All @@ -537,22 +538,29 @@ public static ProtoInfo createProtoInfo(
NestedSet<Artifact> transitiveDescriptorSets =
NestedSetBuilder.fromNestedSet(dependenciesDescriptorSets).add(directDescriptorSet).build();

NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> importableProtos =
computeImportableProtos(ruleContext, library);
NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> exportedProtos =
computeExportedProtos(ruleContext, library);
NestedSet<Pair<PathFragment, ImmutableList<Artifact>>> transitiveProtos =
computeTransitiveProtos(ruleContext, exportedProtos);

ProtoInfo protoInfo =
new ProtoInfo(
library.getSources(),
directProtoSources,
/* directProtoSources */ library.getSources(),
/* originalDirectProtoSources */ directProtoSources,
library.getSourceRoot(),
transitiveProtoSources,
transitiveOriginalProtoSources,
transitiveProtoSourceRoots,
strictImportableProtosForDependents,
strictImportableProtos,
strictImportableProtosImportPathsForDependents,
strictImportableProtoSourceRoots,
exportedProtos,
exportedProtoSourceRoots,
directDescriptorSet,
transitiveDescriptorSets,
importableProtos,
exportedProtos,
transitiveProtos,
Location.BUILTIN);

return protoInfo;
Expand Down Expand Up @@ -646,17 +654,6 @@ private static NestedSet<Pair<Artifact, String>> computeStrictImportableProtos(
return result.build();
}

/**
* Returns the .proto files that are the direct srcs of the exported dependencies of this rule.
*/
private static NestedSet<Pair<Artifact, String>> computeExportedProtos(RuleContext ruleContext) {
NestedSetBuilder<Pair<Artifact, String>> result = NestedSetBuilder.stableOrder();
for (ProtoInfo provider : ruleContext.getPrerequisites("exports", TARGET, ProtoInfo.PROVIDER)) {
result.addTransitive(provider.getStrictImportableProtoSourcesImportPaths());
}
return result.build();
}

/**
* Decides whether this proto_library should check for strict proto deps.
*
Expand Down
Loading

0 comments on commit 7ab5bd3

Please sign in to comment.