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 1463bbfc341129..cbeeaa264809d0 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 @@ -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; @@ -136,10 +137,13 @@ static NestedSet computeDependenciesDescriptorSets(RuleContext ruleCon *

Assumes {@code currentProtoSourceRoot} is the same as the package name. */ private static NestedSet computeTransitiveProtoSourceRoots( - RuleContext ruleContext, String currentProtoSourceRoot) { + RuleContext ruleContext, Optional currentProtoSourceRoot) { NestedSetBuilder 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()); @@ -159,11 +163,11 @@ private static NestedSet computeTransitiveProtoSourceRoots( static final class Library { private final ImmutableList sources; private final ImmutableList> importPathSourcePair; - private final String sourceRoot; + private final Optional sourceRoot; Library( ImmutableList sources, - String sourceRoot, + Optional sourceRoot, ImmutableList> importPathSourcePair) { this.sources = sources; this.sourceRoot = sourceRoot; @@ -178,7 +182,7 @@ public ImmutableList> getImportPathSourcePair() { return importPathSourcePair; } - public String getSourceRoot() { + public Optional getSourceRoot() { return sourceRoot; } } @@ -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> builder = ImmutableList.builder(); for (Artifact protoSource : directSources) { builder.add(new Pair(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( @@ -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 computeImports( @@ -361,9 +381,11 @@ private static Pair computeImports( *

Assumes {@code currentProtoSourceRoot} is the same as the package name. */ private static NestedSet getProtoSourceRootsOfAttribute( - RuleContext ruleContext, String currentProtoSourceRoot, String attributeName) { + RuleContext ruleContext, Optional currentProtoSourceRoot, String attributeName) { NestedSetBuilder protoSourceRoots = NestedSetBuilder.stableOrder(); - protoSourceRoots.add(currentProtoSourceRoot); + if (currentProtoSourceRoot.isPresent()) { + protoSourceRoots.add(currentProtoSourceRoot.get()); + } for (ProtoInfo provider : ruleContext.getPrerequisites(attributeName, Mode.TARGET, ProtoInfo.PROVIDER)) { @@ -380,7 +402,7 @@ private static NestedSet getProtoSourceRootsOfAttribute( *

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

Assumes {@code currentProtoSourceRoot} is the same as the package name. */ private static NestedSet computeExportedProtoSourceRoots( - RuleContext ruleContext, String currentProtoSourceRoot) { + RuleContext ruleContext, Optional currentProtoSourceRoot) { return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "exports"); } @@ -490,7 +512,7 @@ public static ProtoInfo createProtoInfo(RuleContext ruleContext) { ProtoInfo protoInfo = new ProtoInfo( library.getSources(), - library.getSourceRoot(), + library.getSourceRoot().or("."), transitiveProtoSources, transitiveProtoSourceRoots, strictImportableProtosForDependents, diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java index 225134e5187910..447763bf4daeb5 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java @@ -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