diff options
3 files changed, 95 insertions, 90 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java index 13300208e2..89a9577600 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.util.LazyString; -import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; @@ -306,10 +305,10 @@ public class ProtoCompileActionBuilder { boolean areDepsStrict = areDepsStrict(ruleContext); // Add include maps - result.addCustomMultiArgv( - new ProtoCommandLineArgv( - areDepsStrict ? supportData.getProtosInDirectDeps() : null, - supportData.getTransitiveImports())); + addIncludeMapArguments( + result, + areDepsStrict ? supportData.getProtosInDirectDeps() : null, + supportData.getTransitiveImports()); if (areDepsStrict) { // Note: the %s in the line below is used by proto-compiler. That is, the string we create @@ -332,58 +331,8 @@ public class ProtoCompileActionBuilder { return result; } - /** - * Static inner class since these objects live into the execution phase and so they must not keep - * alive references to the surrounding analysis-phase objects. - */ @VisibleForTesting - static class ProtoCommandLineArgv extends CustomCommandLine.CustomMultiArgv { - @Nullable private final Iterable<Artifact> protosInDirectDependencies; - private final Iterable<Artifact> transitiveImports; - - ProtoCommandLineArgv( - @Nullable Iterable<Artifact> protosInDirectDependencies, - Iterable<Artifact> transitiveImports) { - this.protosInDirectDependencies = protosInDirectDependencies; - this.transitiveImports = transitiveImports; - } - - @Override - public Iterable<String> argv() { - ImmutableList.Builder<String> builder = ImmutableList.builder(); - for (Artifact artifact : transitiveImports) { - builder.add( - "-I" + getPathIgnoringRepository(artifact) + "=" + artifact.getExecPathString()); - } - if (protosInDirectDependencies != null) { - ArrayList<String> rootRelativePaths = new ArrayList<>(); - for (Artifact directDependency : protosInDirectDependencies) { - rootRelativePaths.add(getPathIgnoringRepository(directDependency)); - } - builder.add("--direct_dependencies=" + Joiner.on(":").join(rootRelativePaths)); - } - return builder.build(); - } - - /** - * Gets the artifact's path relative to the root, ignoring the external repository the artifact - * is at. For example, <code> - * //a:b.proto --> a/b.proto - * {@literal @}foo//a:b.proto --> a/b.proto - * </code> - */ - private static String getPathIgnoringRepository(Artifact artifact) { - return artifact - .getRootRelativePath() - .relativeTo( - artifact - .getOwnerLabel() - .getPackageIdentifier() - .getRepository() - .getPathUnderExecRoot()) - .toString(); - } - } + static class ProtoCommandLineArgv {} /** Signifies that a prerequisite could not be satisfied. */ private static class MissingPrerequisiteException extends Exception {} @@ -589,7 +538,7 @@ public class ProtoCompileActionBuilder { cmdLine.addAll(protocOpts); // Add include maps - cmdLine.addCustomMultiArgv(new ProtoCommandLineArgv(protosInDirectDeps, transitiveSources)); + addIncludeMapArguments(cmdLine, protosInDirectDeps, transitiveSources); if (protosInDirectDeps != null) { cmdLine.addFormatted(STRICT_DEPS_FLAG_TEMPLATE, ruleLabel); @@ -606,6 +555,45 @@ public class ProtoCompileActionBuilder { return cmdLine.build(); } + @VisibleForTesting + static void addIncludeMapArguments( + CustomCommandLine.Builder commandLine, + @Nullable NestedSet<Artifact> protosInDirectDependencies, + NestedSet<Artifact> transitiveImports) { + commandLine.addAll(transitiveImports, ProtoCompileActionBuilder::transitiveImportArg); + if (protosInDirectDependencies != null) { + if (!protosInDirectDependencies.isEmpty()) { + commandLine.addJoined( + "--direct_dependencies", + ":", + protosInDirectDependencies, + ProtoCompileActionBuilder::getPathIgnoringRepository); + } else { + // The proto compiler requires an empty list to turn on strict deps checking + commandLine.add("--direct_dependencies="); + } + } + } + + private static String transitiveImportArg(Artifact artifact) { + return "-I" + getPathIgnoringRepository(artifact) + "=" + artifact.getExecPathString(); + } + + /** + * Gets the artifact's path relative to the root, ignoring the external repository the artifact is + * at. For example, <code> + * //a:b.proto --> a/b.proto + * {@literal @}foo//a:b.proto --> a/b.proto + * </code> + */ + private static String getPathIgnoringRepository(Artifact artifact) { + return artifact + .getRootRelativePath() + .relativeTo( + artifact.getOwnerLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot()) + .toString(); + } + /** * Describes a toolchain and the value to replace for a $(OUT) that might appear in its * commandLine() (e.g., "bazel-out/foo.srcjar"). 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 ad446803c3..5e4e53cc6a 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 @@ -192,16 +192,20 @@ public class BazelProtoLibraryTest extends BuildViewTestCase { "proto_library(name='dep2', srcs=['dep2.proto'])"); assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x:nodeps")).getRemainingArguments()) - .contains("--direct_dependencies=x/nodeps.proto"); + .containsAllOf("--direct_dependencies", "x/nodeps.proto") + .inOrder(); assertThat( getGeneratingSpawnAction(getDescriptorOutput("//x:withdeps")).getRemainingArguments()) - .contains("--direct_dependencies=x/dep1.proto:x/dep2.proto:x/withdeps.proto"); + .containsAllOf("--direct_dependencies", "x/dep1.proto:x/dep2.proto:x/withdeps.proto") + .inOrder(); assertThat( getGeneratingSpawnAction(getDescriptorOutput("//x:depends_on_alias")) .getRemainingArguments()) - .contains("--direct_dependencies=x/dep1.proto:x/dep2.proto:x/depends_on_alias.proto"); + .containsAllOf( + "--direct_dependencies", "x/dep1.proto:x/dep2.proto:x/depends_on_alias.proto") + .inOrder(); } /** @@ -219,7 +223,8 @@ public class BazelProtoLibraryTest extends BuildViewTestCase { assertThat(file.getRootRelativePathString()).isEqualTo("x/foo-descriptor-set.proto.bin"); assertThat(getGeneratingSpawnAction(file).getRemainingArguments()) - .contains("--direct_dependencies=x/foo.proto:x/bar.proto"); + .containsAllOf("--direct_dependencies", "x/foo.proto:x/bar.proto") + .inOrder(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java index 2cf4e1b36b..44cc3f8418 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java @@ -29,12 +29,13 @@ import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; -import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder.ProtoCommandLineArgv; import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder.ToolchainInvocation; import com.google.devtools.build.lib.util.LazyString; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import javax.annotation.Nullable; import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; @@ -164,7 +165,8 @@ public class ProtoCompileActionBuilderTest { "--java_out=param1,param2:foo.srcjar", "-Iimport1.proto=import1.proto", "-Iimport2.proto=import2.proto", - "--direct_dependencies=import1.proto", + "--direct_dependencies", + "import1.proto", String.format(ProtoCompileActionBuilder.STRICT_DEPS_FLAG_TEMPLATE, "//foo:bar"), "source_file.proto") .inOrder(); @@ -289,35 +291,32 @@ public class ProtoCompileActionBuilderTest { @Test public void testProtoCommandLineArgv() throws Exception { assertThat( - new ProtoCommandLineArgv( - null /* directDependencies */, - ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))) - .argv()) + protoArgv( + null /* directDependencies */, + ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")))) .containsExactly("-Ifoo.proto=out/foo.proto"); assertThat( - new ProtoCommandLineArgv( - ImmutableList.<Artifact>of() /* directDependencies */, - ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))) - .argv()) + protoArgv( + ImmutableList.of() /* directDependencies */, + ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")))) .containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies="); assertThat( - new ProtoCommandLineArgv( - ImmutableList.of( - derivedArtifact("//:dont-care", "foo.proto")) /* directDependencies */, - ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))) - .argv()) - .containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies=foo.proto"); + protoArgv( + ImmutableList.of( + derivedArtifact("//:dont-care", "foo.proto")) /* directDependencies */, + ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")))) + .containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies", "foo.proto"); assertThat( - new ProtoCommandLineArgv( - ImmutableList.of( - derivedArtifact("//:dont-care", "foo.proto"), - derivedArtifact("//:dont-care", "bar.proto")) /* directDependencies */, - ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))) - .argv()) - .containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies=foo.proto:bar.proto"); + protoArgv( + ImmutableList.of( + derivedArtifact("//:dont-care", "foo.proto"), + derivedArtifact("//:dont-care", "bar.proto")) /* directDependencies */, + ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")))) + .containsExactly( + "-Ifoo.proto=out/foo.proto", "--direct_dependencies", "foo.proto:bar.proto"); } /** @@ -329,10 +328,9 @@ public class ProtoCompileActionBuilderTest { @Test public void testIncludeMapsOfExternalFiles() throws Exception { assertThat( - new ProtoCommandLineArgv( - null /* protosInDirectoDependencies */, - ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto"))) - .argv()) + protoArgv( + null /* protosInDirectoDependencies */, + ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto")))) .containsExactly("-Ifoo/bar.proto=external/bla/foo/bar.proto"); } @@ -342,8 +340,7 @@ public class ProtoCompileActionBuilderTest { public void directDependenciesOnExternalFiles() throws Exception { ImmutableList<Artifact> protos = ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto")); - assertThat(new ProtoCommandLineArgv(protos, protos).argv()) - .containsExactly("--direct_dependencies=foo/bar.proto"); + assertThat(protoArgv(protos, protos)).containsExactly("--direct_dependencies", "foo/bar.proto"); } private Artifact artifact(String ownerLabel, String path) { @@ -362,4 +359,19 @@ public class ProtoCompileActionBuilderTest { derivedRoot.getExecPath().getRelative(path), new LabelArtifactOwner(Label.parseAbsoluteUnchecked(ownerLabel))); } + + private static Iterable<String> protoArgv( + @Nullable Iterable<Artifact> protosInDirectDependencies, + Iterable<Artifact> transitiveImports) { + CustomCommandLine.Builder commandLine = CustomCommandLine.builder(); + NestedSet<Artifact> protosInDirectDependenciesBuilder = + protosInDirectDependencies != null + ? NestedSetBuilder.wrap(STABLE_ORDER, protosInDirectDependencies) + : null; + NestedSet<Artifact> transitiveImportsNestedSet = + NestedSetBuilder.wrap(STABLE_ORDER, transitiveImports); + ProtoCompileActionBuilder.addIncludeMapArguments( + commandLine, protosInDirectDependenciesBuilder, transitiveImportsNestedSet); + return commandLine.build().arguments(); + } } |