ProtoCompileActionBuilder takes a list of ToolchainInvocations instead of a map, to emphasize that order matters.

--
MOS_MIGRATED_REVID=138090273

2 files changed, 29 insertions, 13 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
index d3bc185cfb..9908324de7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java
@@ -25,7 +25,6 @@ import static com.google.devtools.build.lib.rules.java.proto.JavaCompilationArgs
 import static com.google.devtools.build.lib.rules.java.proto.JavaProtoLibraryTransitiveFilesToBuildProvider.GET_JARS;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
@@ -200,10 +199,9 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured
     private void createProtoCompileAction(Artifact sourceJar) {
       ProtoCompileActionBuilder.registerActions(
           ruleContext,
-          ImmutableMap.of(
-              "javalite",
+          ImmutableList.of(
               new ProtoCompileActionBuilder.ToolchainInvocation(
-                  getProtoToolchainProvider(), sourceJar.getExecPathString())),
+                  "javalite", getProtoToolchainProvider(), sourceJar.getExecPathString())),
           supportData,
           ImmutableList.of(sourceJar),
           "JavaLite",
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 29b2c4fc37..14b66abadd 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
@@ -35,6 +35,8 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.util.LazyString;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -345,7 +347,7 @@ public class ProtoCompileActionBuilder {
    */
   public static void registerActions(
       RuleContext ruleContext,
-      ImmutableMap<String, ToolchainInvocation> toolchainInvocations,
+      List<ToolchainInvocation> toolchainInvocations,
       SupportData supportData,
       Iterable<Artifact> outputs,
       String flavorName,
@@ -357,7 +359,7 @@ public class ProtoCompileActionBuilder {
     SpawnAction.Builder result =
         new SpawnAction.Builder().addTransitiveInputs(supportData.getTransitiveImports());
 
-    for (ToolchainInvocation invocation : toolchainInvocations.values()) {
+    for (ToolchainInvocation invocation : toolchainInvocations) {
       ProtoLangToolchainProvider toolchain = invocation.toolchain;
       if (toolchain.pluginExecutable() != null) {
         result.addTool(toolchain.pluginExecutable());
@@ -405,19 +407,31 @@ public class ProtoCompileActionBuilder {
    *       --plugin=protoc-gen-PLUGIN_<key>=<location of plugin>.
    * </ul>
    *
+   * Note {@code toolchainInvocations} is ordered, and affects the order in which plugins are
+   * called. As some plugins rely on output from other plugins, their order matters.
+   *
    * @return a command-line to pass to proto-compiler.
    */
   @VisibleForTesting
   static CustomCommandLine createCommandLineFromToolchains(
-      ImmutableMap<String, ToolchainInvocation> toolchainInvocations,
+      List<ToolchainInvocation> toolchainInvocations,
       SupportData supportData,
       boolean allowServices,
       ImmutableList<String> protocOpts) {
     CustomCommandLine.Builder cmdLine = CustomCommandLine.builder();
 
-    for (Map.Entry<String, ToolchainInvocation> entry : toolchainInvocations.entrySet()) {
-      String pluginSuffix = entry.getKey();
-      ToolchainInvocation invocation = entry.getValue();
+    // A set to check if there are multiple invocations with the same name.
+    HashSet<String> invocationNames = new HashSet<>();
+
+    for (ToolchainInvocation invocation : toolchainInvocations) {
+      if (!invocationNames.add(invocation.name)) {
+        throw new IllegalStateException(
+            "Invocation name "
+                + invocation.name
+                + " appears more than once. "
+                + "This could lead to incorrect proto-compiler behavior");
+      }
+
       ProtoLangToolchainProvider toolchain = invocation.toolchain;
 
       cmdLine.add(
@@ -427,13 +441,13 @@ public class ProtoCompileActionBuilder {
                   "OUT",
                   invocation.outReplacement,
                   "PLUGIN_OUT",
-                  String.format("PLUGIN_%s_out", pluginSuffix))));
+                  String.format("PLUGIN_%s_out", invocation.name))));
 
       if (toolchain.pluginExecutable() != null) {
         cmdLine.add(
             String.format(
                 "--plugin=protoc-gen-%s=%s",
-                String.format("PLUGIN_%s", pluginSuffix),
+                String.format("PLUGIN_%s", invocation.name),
                 toolchain.pluginExecutable().getExecutable().getExecPathString()));
       }
     }
@@ -459,10 +473,14 @@ public class ProtoCompileActionBuilder {
    * commandLine() (e.g., "bazel-out/foo.srcjar").
    */
   public static class ToolchainInvocation {
+    final String name;
     public final ProtoLangToolchainProvider toolchain;
     final CharSequence outReplacement;
 
-    public ToolchainInvocation(ProtoLangToolchainProvider toolchain, CharSequence outReplacement) {
+    public ToolchainInvocation(
+        String name, ProtoLangToolchainProvider toolchain, CharSequence outReplacement) {
+      checkState(!name.contains(" "), "Name %s should not contain spaces", name);
+      this.name = name;
       this.toolchain = toolchain;
       this.outReplacement = outReplacement;
     }