Use CustomCommandLine directly instead of via SpawnAction.Builder.

This change forces use of CustomCommandLine.Builder, which has a richer interface for constructing memory-efficient command lines. It will also permit surveying the code base for inefficient patterns using an IDE.

This change was done by hand and split using Rosie to assist with rollbacks in case of bugs. Reviewers, please pay particular attention to:

* Each all to addInputArgument/addOutputArgument should come with a corresponding matching pair to SpawnAction.Builder#addInput and CustomCommandLine.Builder#addExecPath (eg.).
* The commandLine must be set on the SpawnAction using SpawnAction.Builder#setCommandLine.

Note that most calls to addPrefixed("arg=", val) should be more idiomatically expressed as add("arg", val), but this involves changing tests and making sure that the command line tools can accept the format.

PiperOrigin-RevId: 166249211

3 files changed, 152 insertions, 109 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
index dda684a015..a73ddde836 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java
@@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 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.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.rules.android.AndroidCommon;
@@ -73,8 +74,10 @@ public class BazelAndroidSemantics implements AndroidSemantics {
 
   @Override
   public void addMainDexListActionArguments(
-      RuleContext ruleContext, SpawnAction.Builder builder, Artifact proguardMap) {
-  }
+      RuleContext ruleContext,
+      SpawnAction.Builder builder,
+      CustomCommandLine.Builder commandLine,
+      Artifact proguardMap) {}
 
   @Override
   public Artifact getApkDebugSigningKey(RuleContext ruleContext) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index c6f99e0cef..18f7835010 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -509,29 +509,33 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     if (rexEnabled) {
       finalDexes = getDxArtifact(ruleContext, "rexed_dexes.zip");
       Builder rexActionBuilder = new SpawnAction.Builder();
+      CustomCommandLine.Builder commandLine = CustomCommandLine.builder();
       rexActionBuilder
           .setExecutable(ruleContext.getExecutablePrerequisite("$rex_wrapper", Mode.HOST))
           .setMnemonic("Rex")
           .setProgressMessage("Rexing dex files")
-          .addArgument("--dex_input")
-          .addInputArgument(dexingOutput.classesDexZip)
-          .addArgument("--dex_output")
-          .addOutputArgument(finalDexes);
+          .addInput(dexingOutput.classesDexZip)
+          .addOutput(finalDexes);
+      commandLine
+          .addExecPath("--dex_input", dexingOutput.classesDexZip)
+          .addExecPath("--dex_output", finalDexes);
       if (proguardOutput.getMapping() != null) {
         finalProguardMap =
             ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_PROGUARD_MAP);
         Artifact finalRexPackageMap = getDxArtifact(ruleContext, "rex_output_package.map");
         rexActionBuilder
-            .addArgument("--proguard_input_map")
-            .addInputArgument(proguardOutput.getMapping())
-            .addArgument("--proguard_output_map")
-            .addOutputArgument(finalProguardMap)
-            .addArgument("--rex_output_package_map")
-            .addOutputArgument(finalRexPackageMap);
+            .addInput(proguardOutput.getMapping())
+            .addOutput(finalProguardMap)
+            .addOutput(finalRexPackageMap);
+        commandLine
+            .addExecPath("--proguard_input_map", proguardOutput.getMapping())
+            .addExecPath("--proguard_output_map", finalProguardMap)
+            .addExecPath("--rex_output_package_map", finalRexPackageMap);
         if (ruleContext.attributes().isAttributeValueExplicitlySpecified("rex_package_map")) {
           Artifact rexPackageMap =
               ruleContext.getPrerequisiteArtifact("rex_package_map", Mode.TARGET);
-          rexActionBuilder.addArgument("--rex_input_package_map").addInputArgument(rexPackageMap);
+          rexActionBuilder.addInput(rexPackageMap);
+          commandLine.addExecPath("--rex_input_package_map", rexPackageMap);
         }
       } else {
         finalProguardMap = proguardOutput.getMapping();
@@ -542,8 +546,9 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       // when multidex mode is set to legacy.
       if (ruleContext.attributes().isAttributeValueExplicitlySpecified("main_dex_list")
           || getMultidexMode(ruleContext) == MultidexMode.LEGACY) {
-        rexActionBuilder.addArgument("--keep-main-dex");
+        commandLine.add("--keep-main-dex");
       }
+      rexActionBuilder.setCommandLine(commandLine.build());
       ruleContext.registerAction(rexActionBuilder.build(ruleContext));
     } else {
       finalDexes = dexingOutput.classesDexZip;
@@ -616,9 +621,14 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
                 "Generating incremental installation manifest for %s", ruleContext.getLabel())
             .setExecutable(
                 ruleContext.getExecutablePrerequisite("$build_incremental_dexmanifest", Mode.HOST))
-            .addOutputArgument(incrementalDexManifest)
-            .addInputArguments(dexingOutput.shardDexZips)
+            .addOutput(incrementalDexManifest)
+            .addInputs(dexingOutput.shardDexZips)
             .useParameterFile(ParameterFileType.UNQUOTED)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .addExecPath(incrementalDexManifest)
+                    .addExecPaths(dexingOutput.shardDexZips)
+                    .build())
             .build(ruleContext));
 
     Artifact stubData = ruleContext.getImplicitOutputArtifact(
@@ -732,15 +742,19 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     splitApkSetBuilder.add(javaSplitApk);
 
     Artifact splitMainApkResources = getDxArtifact(ruleContext, "split_main.ap_");
-    ruleContext.registerAction(new SpawnAction.Builder()
-        .setMnemonic("AndroidStripResources")
-        .setProgressMessage("Stripping resources from split main apk")
-        .setExecutable(ruleContext.getExecutablePrerequisite("$strip_resources", Mode.HOST))
-        .addArgument("--input_resource_apk")
-        .addInputArgument(resourceApk.getArtifact())
-        .addArgument("--output_resource_apk")
-        .addOutputArgument(splitMainApkResources)
-        .build(ruleContext));
+    ruleContext.registerAction(
+        new SpawnAction.Builder()
+            .setMnemonic("AndroidStripResources")
+            .setProgressMessage("Stripping resources from split main apk")
+            .setExecutable(ruleContext.getExecutablePrerequisite("$strip_resources", Mode.HOST))
+            .addInput(resourceApk.getArtifact())
+            .addOutput(splitMainApkResources)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .addExecPath("--input_resource_apk", resourceApk.getArtifact())
+                    .addExecPath("--output_resource_apk", splitMainApkResources)
+                    .build())
+            .build(ruleContext));
 
     NestedSet<Artifact> splitApks = splitApkSetBuilder.build();
     Artifact splitMainApk = getDxArtifact(ruleContext, "split_main.apk");
@@ -869,24 +883,25 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
             .setMnemonic("AndroidInstall")
             .setProgressMessage("Installing %s using split apks", ruleContext.getLabel())
             .setExecutionInfo(ImmutableMap.of("local", ""))
-            .addArgument("--output_marker")
-            .addOutputArgument(marker)
-            .addArgument("--stub_datafile")
-            .addInputArgument(stubDataFile)
-            .addArgument("--adb")
-            .addArgument(adb.getExecutable().getExecPathString())
             .addTool(adb)
-            .addArgument("--flagfile")
-            .addInputArgument(argsArtifact)
-            .addArgument("--split_main_apk")
-            .addInputArgument(splitMainApk);
+            .addOutput(marker)
+            .addInput(stubDataFile)
+            .addInput(argsArtifact)
+            .addInput(splitMainApk);
+    CustomCommandLine.Builder commandLine =
+        CustomCommandLine.builder()
+            .addExecPath("--output_marker", marker)
+            .addExecPath("--stub_datafile", stubDataFile)
+            .addExecPath("--adb", adb.getExecutable())
+            .addExecPath("--flagfile", argsArtifact)
+            .addExecPath("--split_main_apk", splitMainApk);
 
     for (Artifact splitApk : splitApks) {
-      builder
-          .addArgument("--split_apk")
-          .addInputArgument(splitApk);
+      builder.addInput(splitApk);
+      commandLine.addExecPath("--split_apk", splitApk);
     }
 
+    builder.setCommandLine(commandLine.build());
     ruleContext.registerAction(builder.build(ruleContext));
   }
 
@@ -906,37 +921,37 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
             .setProgressMessage(
                 "Installing %s%s", ruleContext.getLabel(), (incremental ? " incrementally" : ""))
             .setExecutionInfo(ImmutableMap.of("local", ""))
-            .addArgument("--output_marker")
-            .addOutputArgument(marker)
-            .addArgument("--dexmanifest")
-            .addInputArgument(dexmanifest)
-            .addArgument("--resource_apk")
-            .addInputArgument(resourceApk)
-            .addArgument("--stub_datafile")
-            .addInputArgument(stubDataFile)
-            .addArgument("--adb")
-            .addArgument(adb.getExecutable().getExecPathString())
             .addTool(adb)
-            .addArgument("--flagfile")
-            .addInputArgument(argsArtifact);
+            .addOutput(marker)
+            .addInput(dexmanifest)
+            .addInput(resourceApk)
+            .addInput(stubDataFile)
+            .addInput(argsArtifact);
+
+    CustomCommandLine.Builder commandLine =
+        CustomCommandLine.builder()
+            .addExecPath("--output_marker", marker)
+            .addExecPath("--dexmanifest", dexmanifest)
+            .addExecPath("--resource_apk", resourceApk)
+            .addExecPath("--stub_datafile", stubDataFile)
+            .addExecPath("--adb", adb.getExecutable())
+            .addExecPath("--flagfile", argsArtifact);
 
     if (!incremental) {
-      builder
-          .addArgument("--apk")
-          .addInputArgument(apk);
+      builder.addInput(apk);
+      commandLine.addExecPath("--apk", apk);
     }
 
     if (ruleContext.getFragment(AndroidConfiguration.class).useIncrementalNativeLibs()) {
       for (Map.Entry<String, NestedSet<Artifact>> arch : nativeLibs.getMap().entrySet()) {
         for (Artifact lib : arch.getValue()) {
-          builder
-              .addArgument("--native_lib")
-              .addArgument(arch.getKey() + ":" + lib.getExecPathString())
-              .addInput(lib);
+          builder.addInput(lib);
+          commandLine.add("--native_lib").addFormatted("%s:%s", arch.getKey(), lib);
         }
       }
     }
 
+    builder.setCommandLine(commandLine.build());
     ruleContext.registerAction(builder.build(ruleContext));
   }
 
@@ -1440,17 +1455,21 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     SpawnAction.Builder dexmerger =
         new SpawnAction.Builder()
             .setExecutable(ruleContext.getExecutablePrerequisite("$dexmerger", Mode.HOST))
-            .addArgument("--input")
-            .addInputArgument(inputJar)
-            .addArgument("--output")
-            .addOutputArgument(classesDex)
-            .addArguments(DexArchiveAspect.mergerDexopts(ruleContext, dexopts))
-            .addArgument("--multidex=" + multidexStrategy)
             .setMnemonic("DexMerger")
-            .setProgressMessage("Assembling dex files into %s", classesDex.prettyPrint());
+            .setProgressMessage("Assembling dex files into %s", classesDex.getRootRelativePath())
+            .addInput(inputJar)
+            .addOutput(classesDex);
+    CustomCommandLine.Builder commandLine =
+        CustomCommandLine.builder()
+            .addExecPath("--input", inputJar)
+            .addExecPath("--output", classesDex)
+            .addAll(DexArchiveAspect.mergerDexopts(ruleContext, dexopts))
+            .addPrefixed("--multidex=", multidexStrategy);
     if (mainDexList != null) {
-      dexmerger.addArgument("--main-dex-list").addInputArgument(mainDexList);
+      dexmerger.addInput(mainDexList);
+      commandLine.addExecPath("--main-dex-list", mainDexList);
     }
+    dexmerger.setCommandLine(commandLine.build());
     ruleContext.registerAction(dexmerger.build(ruleContext));
   }
 
@@ -1659,16 +1678,19 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       RuleContext ruleContext, Artifact inputZip, Artifact outputZip) {
     ruleContext.registerAction(
         singleJarSpawnActionBuilder(ruleContext)
-            .addArgument("--exclude_build_data")
-            .addArgument("--dont_change_compression")
-            .addArgument("--sources")
-            .addInputArgument(inputZip)
-            .addArgument("--output")
-            .addOutputArgument(outputZip)
-            .addArgument("--include_prefixes")
-            .addArgument("classes")
             .setProgressMessage("Trimming %s", inputZip.getExecPath().getBaseName())
             .setMnemonic("TrimDexZip")
+            .addInput(inputZip)
+            .addOutput(outputZip)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .add("--exclude_build_data")
+                    .add("--dont_change_compression")
+                    .addExecPath("--sources", inputZip)
+                    .addExecPath("--output", outputZip)
+                    .add("--include_prefixes")
+                    .add("classes")
+                    .build())
             .build(ruleContext));
   }
 
@@ -1687,23 +1709,25 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     // Process the input jar through Proguard into an intermediate, streamlined jar.
     Artifact strippedJar = AndroidBinary.getDxArtifact(ruleContext, "main_dex_intermediate.jar");
     AndroidSdkProvider sdk = AndroidSdkProvider.fromRuleContext(ruleContext);
-    SpawnAction.Builder streamlinedBuilder = new SpawnAction.Builder()
-        .addOutput(strippedJar)
-        .setExecutable(sdk.getProguard())
-        .setProgressMessage("Generating streamlined input jar for main dex classes list")
-        .setMnemonic("MainDexClassesIntermediate")
-        .addArgument("-forceprocessing")
-        .addArgument("-injars")
-        .addInputArgument(jar)
-        .addArgument("-libraryjars")
-        .addInputArgument(sdk.getShrinkedAndroidJar())
-        .addArgument("-outjars")
-        .addArgument(strippedJar.getExecPathString())
-        .addArgument("-dontwarn")
-        .addArgument("-dontnote")
-        .addArgument("-dontoptimize")
-        .addArgument("-dontobfuscate")
-        .addArgument("-dontpreverify");
+    SpawnAction.Builder streamlinedBuilder =
+        new SpawnAction.Builder()
+            .addOutput(strippedJar)
+            .setExecutable(sdk.getProguard())
+            .setProgressMessage("Generating streamlined input jar for main dex classes list")
+            .setMnemonic("MainDexClassesIntermediate")
+            .addInput(jar)
+            .addInput(sdk.getShrinkedAndroidJar());
+    CustomCommandLine.Builder streamlinedCommandLine =
+        CustomCommandLine.builder()
+            .add("-forceprocessing")
+            .addExecPath("-injars", jar)
+            .addExecPath("-libraryjars", sdk.getShrinkedAndroidJar())
+            .addExecPath("-outjars", strippedJar)
+            .add("-dontwarn")
+            .add("-dontnote")
+            .add("-dontoptimize")
+            .add("-dontobfuscate")
+            .add("-dontpreverify");
 
     List<Artifact> specs = new ArrayList<>();
     specs.addAll(
@@ -1716,13 +1740,14 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
     }
 
     for (Artifact spec : specs) {
-      streamlinedBuilder.addArgument("-include");
-      streamlinedBuilder.addInputArgument(spec);
+      streamlinedBuilder.addInput(spec);
+      streamlinedCommandLine.addExecPath("-include", spec);
     }
 
-    androidSemantics
-        .addMainDexListActionArguments(ruleContext, streamlinedBuilder, proguardOutputMap);
+    androidSemantics.addMainDexListActionArguments(
+        ruleContext, streamlinedBuilder, streamlinedCommandLine, proguardOutputMap);
 
+    streamlinedBuilder.setCommandLine(streamlinedCommandLine.build());
     ruleContext.registerAction(streamlinedBuilder.build(ruleContext));
 
     // Create the main dex classes list.
@@ -1731,13 +1756,20 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         .setMnemonic("MainDexClasses")
         .setProgressMessage("Generating main dex classes list");
 
-    ruleContext.registerAction(builder
-        .setExecutable(sdk.getMainDexListCreator())
-        .addOutputArgument(mainDexList)
-        .addInputArgument(strippedJar)
-        .addInputArgument(jar)
-        .addArguments(ruleContext.getTokenizedStringListAttr("main_dex_list_opts"))
-        .build(ruleContext));
+    ruleContext.registerAction(
+        builder
+            .setExecutable(sdk.getMainDexListCreator())
+            .addOutput(mainDexList)
+            .addInput(strippedJar)
+            .addInput(jar)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .addExecPath(mainDexList)
+                    .addExecPath(strippedJar)
+                    .addExecPath(jar)
+                    .addAll(ruleContext.getTokenizedStringListAttr("main_dex_list_opts"))
+                    .build())
+            .build(ruleContext));
     return mainDexList;
   }
 
@@ -1752,13 +1784,16 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
             .setExecutable(sdk.getAapt())
             .setMnemonic("AaptSplitResourceApk")
             .setProgressMessage("Generating resource apk for split %s", splitName)
-            .addArgument("package")
-            .addArgument("-F")
-            .addOutputArgument(splitResources)
-            .addArgument("-M")
-            .addInputArgument(splitManifest)
-            .addArgument("-I")
-            .addInputArgument(sdk.getAndroidJar())
+            .addOutput(splitResources)
+            .addInput(splitManifest)
+            .addInput(sdk.getAndroidJar())
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .add("package")
+                    .addExecPath("-F", splitResources)
+                    .addExecPath("-M", splitManifest)
+                    .addExecPath("-I", sdk.getAndroidJar())
+                    .build())
             .build(ruleContext));
 
     return splitResources;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
index 76fd3e087c..f7637a7e3f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java
@@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.OutputGroupProvider;
 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.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArtifacts;
@@ -73,10 +74,14 @@ public interface AndroidSemantics {
 
   /**
    * Configures the builder for generating the output jar used to configure the main dex file.
+   *
    * @throws InterruptedException
    */
   void addMainDexListActionArguments(
-      RuleContext ruleContext, SpawnAction.Builder builder, Artifact proguardMap)
+      RuleContext ruleContext,
+      SpawnAction.Builder builder,
+      CustomCommandLine.Builder commandLine,
+      Artifact proguardMap)
       throws InterruptedException;
 
   /**