Skip to content

Commit

Permalink
Do not add a --proto_path argument to the proto compiler invocation t…
Browse files Browse the repository at this point in the history 
…here are no .proto sources in it.

This prevents protoc from complaining about a nonexistent directory in sandboxed mode and is good hygiene in any case.

Fixes bazelbuild#7157.

RELNOTES: None.
PiperOrigin-RevId: 256208923
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Jul 2, 2019
1 parent 316cb9b commit 5c83cd9
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 24 deletions.
66 changes: 44 additions & 22 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 @@ -19,6 +19,7 @@
import static com.google.devtools.build.lib.syntax.Type.STRING;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
Expand Down Expand Up @@ -136,10 +137,13 @@ static NestedSet<Artifact> computeDependenciesDescriptorSets(RuleContext ruleCon
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
private static NestedSet<String> computeTransitiveProtoSourceRoots(
RuleContext ruleContext, String currentProtoSourceRoot) {
RuleContext ruleContext, Optional<String> currentProtoSourceRoot) {
NestedSetBuilder<String> protoPath = NestedSetBuilder.stableOrder();

protoPath.add(currentProtoSourceRoot);
if (currentProtoSourceRoot.isPresent()) {
protoPath.add(currentProtoSourceRoot.get());
}

for (ProtoInfo provider :
ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoInfo.PROVIDER)) {
protoPath.addTransitive(provider.getTransitiveProtoSourceRoots());
Expand All @@ -159,11 +163,11 @@ private static NestedSet<String> computeTransitiveProtoSourceRoots(
static final class Library {
private final ImmutableList<Artifact> sources;
private final ImmutableList<Pair<Artifact, String>> importPathSourcePair;
private final String sourceRoot;
private final Optional<String> sourceRoot;

Library(
ImmutableList<Artifact> sources,
String sourceRoot,
Optional<String> sourceRoot,
ImmutableList<Pair<Artifact, String>> importPathSourcePair) {
this.sources = sources;
this.sourceRoot = sourceRoot;
Expand All @@ -178,7 +182,7 @@ public ImmutableList<Pair<Artifact, String>> getImportPathSourcePair() {
return importPathSourcePair;
}

public String getSourceRoot() {
public Optional<String> getSourceRoot() {
return sourceRoot;
}
}
Expand All @@ -201,22 +205,38 @@ private static Library getProtoSourceRoot(
return null;
}

// This is the same as getPackageIdentifier().getPathUnderExecRoot() due to the check above for
// protoSourceRoot == package name, but it's a bit more future-proof.
String result =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getPathUnderExecRoot()
.getRelative(protoSourceRoot)
.getPathString();
boolean hasNonGeneratedSources = false;
for (Artifact artifact : directSources) {
if (artifact.isSourceArtifact()) {
hasNonGeneratedSources = true;
break;
}
}

String sourceRoot = "";
// Otherwise, we'll direct the proto compiler to the source files using -Ifoo=bar arguments.
// Then let's not spam the command line.
if (hasNonGeneratedSources) {
// This is the same as getPackageIdentifier().getPathUnderExecRoot() due to the check above
// for protoSourceRoot == package name, but it's a bit more future-proof.
sourceRoot =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getPathUnderExecRoot()
.getRelative(protoSourceRoot)
.getPathString();
}

ImmutableList.Builder<Pair<Artifact, String>> builder = ImmutableList.builder();
for (Artifact protoSource : directSources) {
builder.add(new Pair<Artifact, String>(protoSource, null));
}
return new Library(directSources, result.isEmpty() ? "." : result, builder.build());
return new Library(
directSources,
sourceRoot.isEmpty() ? Optional.absent() : Optional.of(sourceRoot),
builder.build());
}

private static PathFragment getPathFragmentAttribute(
Expand Down Expand Up @@ -327,7 +347,7 @@ private static Library createVirtualImportDirectoryMaybe(
.getExecPath()
.getRelative(sourceRootPath)
.getPathString();
return new Library(symlinks.build(), sourceRoot, protoSourceImportPair.build());
return new Library(symlinks.build(), Optional.of(sourceRoot), protoSourceImportPair.build());
}

private static Pair<PathFragment, Artifact> computeImports(
Expand Down Expand Up @@ -361,9 +381,11 @@ private static Pair<PathFragment, Artifact> computeImports(
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
private static NestedSet<String> getProtoSourceRootsOfAttribute(
RuleContext ruleContext, String currentProtoSourceRoot, String attributeName) {
RuleContext ruleContext, Optional<String> currentProtoSourceRoot, String attributeName) {
NestedSetBuilder<String> protoSourceRoots = NestedSetBuilder.stableOrder();
protoSourceRoots.add(currentProtoSourceRoot);
if (currentProtoSourceRoot.isPresent()) {
protoSourceRoots.add(currentProtoSourceRoot.get());
}

for (ProtoInfo provider :
ruleContext.getPrerequisites(attributeName, Mode.TARGET, ProtoInfo.PROVIDER)) {
Expand All @@ -380,7 +402,7 @@ private static NestedSet<String> getProtoSourceRootsOfAttribute(
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
private static NestedSet<String> computeStrictImportableProtoSourceRoots(
RuleContext ruleContext, String currentProtoSourceRoot) {
RuleContext ruleContext, Optional<String> currentProtoSourceRoot) {
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "deps");
}

Expand All @@ -391,7 +413,7 @@ private static NestedSet<String> computeStrictImportableProtoSourceRoots(
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
private static NestedSet<String> computeExportedProtoSourceRoots(
RuleContext ruleContext, String currentProtoSourceRoot) {
RuleContext ruleContext, Optional<String> currentProtoSourceRoot) {
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "exports");
}

Expand Down Expand Up @@ -490,7 +512,7 @@ public static ProtoInfo createProtoInfo(RuleContext ruleContext) {
ProtoInfo protoInfo =
new ProtoInfo(
library.getSources(),
library.getSourceRoot(),
library.getSourceRoot().or("."),
transitiveProtoSources,
transitiveProtoSourceRoots,
strictImportableProtosForDependents,
Expand Down
30 changes: 28 additions & 2 deletions src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,34 @@ public void testProtoSourceRootWithDeps() throws Exception {
);
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly("x/foo", "x/bar", ".");
assertThat(sourcesProvider.getTransitiveProtoSourceRoots()).containsExactly("x/foo", "x/bar");
}

@Test
public void testExternalRepoWithGeneratedProto() throws Exception {
if (!isThisBazel()) {
return;
}

FileSystemUtils.appendIsoLatin1(
scratch.resolve("WORKSPACE"), "local_repository(name = 'foo', path = '/foo')");
invalidatePackages();

scratch.file("/foo/WORKSPACE");
scratch.file(
"/foo/x/BUILD",
"proto_library(name='x', srcs=['generated.proto'])",
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')");

scratch.file("a/BUILD", "proto_library(name='a', srcs=['a.proto'], deps=['@foo//x:x'])");

ConfiguredTarget a = getConfiguredTarget("//a:a");
ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);
assertThat(aInfo.getTransitiveProtoSourceRoots()).isEmpty();

ConfiguredTarget x = getConfiguredTarget("@foo//x:x");
ProtoInfo xInfo = x.get(ProtoInfo.PROVIDER);
assertThat(xInfo.getTransitiveProtoSourceRoots()).isEmpty();
}

@Test
Expand Down

0 comments on commit 5c83cd9

Please sign in to comment.