Stop using CustomMultiArgv in ProtoCompileActionBuilder.
CustomMultiArgv is hard to serialise. This CL still uses mapping functions, which are equally hard to serialise, but it moves one step closer to being structural.
PiperOrigin-RevId: 166230153
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 1330020..89a9577 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.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 @@
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 @@
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 @@
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 @@
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 ad44680..5e4e53c 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 @@
"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 @@
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 2cf4e1b..44cc3f8 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.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 @@
"--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 @@
@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 @@
@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 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 @@
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();
+ }
}
