Skip to content

Commit

Permalink
Automated rollback of commit 5c83cd9.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Breaks Bazel downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1067#24a01f98-1ed2-4423-8d94-bf11cef557e4

*** Original change description ***

Do not add a --proto_path argument to the proto compiler invocation there 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 #7157.

RELNOTES: None.
PiperOrigin-RevId: 256338118
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Jul 3, 2019
1 parent 01e6e05 commit 1a3d4c4
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 72 deletions.
66 changes: 22 additions & 44 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,7 +19,6 @@
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 @@ -137,13 +136,10 @@ static NestedSet<Artifact> computeDependenciesDescriptorSets(RuleContext ruleCon
* <p>Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
private static NestedSet<String> computeTransitiveProtoSourceRoots(
RuleContext ruleContext, Optional<String> currentProtoSourceRoot) {
RuleContext ruleContext, String currentProtoSourceRoot) {
NestedSetBuilder<String> protoPath = NestedSetBuilder.stableOrder();

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

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

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

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

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();
}
// 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();

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

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

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

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

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

Expand Down Expand Up @@ -512,7 +490,7 @@ public static ProtoInfo createProtoInfo(RuleContext ruleContext) {
ProtoInfo protoInfo =
new ProtoInfo(
library.getSources(),
library.getSourceRoot().or("."),
library.getSourceRoot(),
transitiveProtoSources,
transitiveProtoSourceRoots,
strictImportableProtosForDependents,
Expand Down
30 changes: 2 additions & 28 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,34 +340,8 @@ 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");
}

@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();
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly("x/foo", "x/bar", ".");
}

@Test
Expand Down

0 comments on commit 1a3d4c4

Please sign in to comment.