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: 166076054

1 files changed, 104 insertions, 112 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
index d985e16c8e..e143e75ecd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
+import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.rules.android.AndroidConfiguration.ApkSigningMethod;
 import com.google.devtools.build.lib.rules.java.JavaCommon;
@@ -102,8 +103,7 @@ public class ApkActionsBuilder {
    * Adds an individual resource file to the root directory of the APK.
    *
    * <p>This provides the same functionality as {@code javaResourceZip}, except much more hacky.
-   * Will most probably won't work if there is an input artifact in the same directory as this
-   * file.
+   * Will most probably won't work if there is an input artifact in the same directory as this file.
    */
   public ApkActionsBuilder setJavaResourceFile(Artifact javaResourceFile) {
     this.javaResourceFile = javaResourceFile;
@@ -141,9 +141,10 @@ public class ApkActionsBuilder {
 
     // If the caller did not request an unsigned APK, we still need to construct one so that
     // we can sign it. So we make up an intermediate artifact.
-    Artifact intermediateUnsignedApk = unsignedApk != null
-        ? unsignedApk
-        : AndroidBinary.getDxArtifact(ruleContext, "unsigned_" + signedApk.getFilename());
+    Artifact intermediateUnsignedApk =
+        unsignedApk != null
+            ? unsignedApk
+            : AndroidBinary.getDxArtifact(ruleContext, "unsigned_" + signedApk.getFilename());
     if (useSingleJarApkBuilder) {
       buildApk(ruleContext, intermediateUnsignedApk);
     } else {
@@ -175,12 +176,12 @@ public class ApkActionsBuilder {
             .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getApkBuilder())
             .setProgressMessage("Generating unsigned %s", apkName)
             .setMnemonic("AndroidApkBuilder")
-            .addOutputArgument(outApk);
+            .addOutput(outApk);
+    CustomCommandLine.Builder commandLine = CustomCommandLine.builder().addExecPath(outApk);
 
     if (javaResourceZip != null) {
-      actionBuilder
-          .addArgument("-rj")
-          .addInputArgument(javaResourceZip);
+      actionBuilder.addInput(javaResourceZip);
+      commandLine.add("-rj").addExecPath(javaResourceZip);
     }
 
     Pair<Artifact, Runfiles> nativeSymlinksManifestAndRunfiles =
@@ -192,42 +193,43 @@ public class ApkActionsBuilder {
       actionBuilder
           .addRunfilesSupplier(
               new RunfilesSupplierImpl(
-                  nativeSymlinksDir,
-                  nativeSymlinksRunfiles,
-                  nativeSymlinksManifest))
+                  nativeSymlinksDir, nativeSymlinksRunfiles, nativeSymlinksManifest))
           .addInput(nativeSymlinksManifest)
-          .addInputs(nativeLibs.getAllNativeLibs())
-          .addArgument("-nf")
+          .addInputs(nativeLibs.getAllNativeLibs());
+      commandLine
+          .add("-nf")
           // If the native libs are "foo/bar/x86/foo.so", we need to pass "foo/bar" here
-          .addArgument(nativeSymlinksDir.getPathString());
+          .addPath(nativeSymlinksDir);
     }
 
     if (nativeLibs.getName() != null) {
-      actionBuilder
-          .addArgument("-rf")
-          .addArgument(nativeLibs.getName().getExecPath().getParentDirectory().getPathString())
-          .addInput(nativeLibs.getName());
+      actionBuilder.addInput(nativeLibs.getName());
+      commandLine.add("-rf").addPath(nativeLibs.getName().getExecPath().getParentDirectory());
     }
 
     if (javaResourceFile != null) {
-      actionBuilder
-          .addArgument("-rf")
-          .addArgument((javaResourceFile.getExecPath().getParentDirectory().getPathString()))
-          .addInput(javaResourceFile);
+      actionBuilder.addInput(javaResourceFile);
+      commandLine.add("-rf").addPath(javaResourceFile.getExecPath().getParentDirectory());
     }
 
-    actionBuilder.addArgument("-u");
+    commandLine.add("-u");
 
     for (Artifact inputZip : inputZips.build()) {
-      actionBuilder.addArgument("-z").addInputArgument(inputZip);
+      actionBuilder.addInput(inputZip);
+      commandLine.addExecPath("-z", inputZip);
     }
 
     if (classesDex != null) {
-      actionBuilder
-          .addArgument(classesDex.getFilename().endsWith(".dex") ? "-f" : "-z")
-          .addInputArgument(classesDex);
+      actionBuilder.addInput(classesDex);
+      if (classesDex.getFilename().endsWith(".dex")) {
+        commandLine.add("-f");
+      } else {
+        commandLine.add("-z");
+      }
+      commandLine.addExecPath(classesDex);
     }
 
+    actionBuilder.setCommandLine(commandLine.build());
     ruleContext.registerAction(actionBuilder.build(ruleContext));
   }
 
@@ -239,48 +241,39 @@ public class ApkActionsBuilder {
         new SpawnAction.Builder()
             .setMnemonic("ApkBuilder")
             .setProgressMessage("Generating unsigned %s", apkName)
-            .addArgument("--exclude_build_data")
-            .addArgument("--compression")
-            .addArgument("--normalize")
-            .addArgument("--output")
-            .addOutputArgument(compressedApk);
+            .addOutput(compressedApk);
+    CustomCommandLine.Builder compressedApkCommandLine =
+        CustomCommandLine.builder()
+            .add("--exclude_build_data")
+            .add("--compression")
+            .add("--normalize")
+            .addExecPath("--output", compressedApk);
     setSingleJarAsExecutable(ruleContext, compressedApkActionBuilder);
 
     if (classesDex != null) {
+      compressedApkActionBuilder.addInput(classesDex);
       if (classesDex.getFilename().endsWith(".zip")) {
-        compressedApkActionBuilder
-            .addArgument("--sources")
-            .addInputArgument(classesDex);
+        compressedApkCommandLine.addExecPath("--sources", classesDex);
       } else {
-        compressedApkActionBuilder
-            .addInput(classesDex)
-            .addArgument("--resources")
-            .addArgument(
-                singleJarResourcesArgument(
-                    classesDex.getExecPathString(),
-                    classesDex.getFilename()));
+        compressedApkCommandLine
+            .add("--resources")
+            .addFormatted("%s:%s", classesDex, classesDex.getFilename());
       }
     }
 
     if (javaResourceFile != null) {
-      compressedApkActionBuilder
-          .addInput(javaResourceFile)
-          .addArgument("--resources")
-          .addArgument(
-              singleJarResourcesArgument(
-                  javaResourceFile.getExecPathString(),
-                  javaResourceFile.getFilename()));
+      compressedApkActionBuilder.addInput(javaResourceFile);
+      compressedApkCommandLine
+          .add("--resources")
+          .addFormatted("%s:%s", javaResourceFile, javaResourceFile.getFilename());
     }
 
     for (String architecture : nativeLibs.getMap().keySet()) {
       for (Artifact nativeLib : nativeLibs.getMap().get(architecture)) {
-        compressedApkActionBuilder
-            .addArgument("--resources")
-            .addArgument(
-                singleJarResourcesArgument(
-                    nativeLib.getExecPathString(),
-                    "lib/" + architecture + "/" + nativeLib.getFilename()))
-            .addInput(nativeLib);
+        compressedApkActionBuilder.addInput(nativeLib);
+        compressedApkCommandLine
+            .add("--resources")
+            .addFormatted("%s:lib/%s/%s", nativeLib, architecture, nativeLib.getFilename());
       }
     }
 
@@ -288,13 +281,15 @@ public class ApkActionsBuilder {
         new SpawnAction.Builder()
             .setMnemonic("ApkBuilder")
             .setProgressMessage("Generating unsigned %s", apkName)
-            .addArgument("--exclude_build_data")
-            .addArgument("--dont_change_compression")
-            .addArgument("--normalize")
-            .addArgument("--sources")
-            .addInputArgument(compressedApk)
-            .addArgument("--output")
-            .addOutputArgument(outApk);
+            .addInput(compressedApk)
+            .addOutput(outApk);
+    CustomCommandLine.Builder singleJarCommandLine = CustomCommandLine.builder();
+    singleJarCommandLine
+        .add("--exclude_build_data")
+        .add("--dont_change_compression")
+        .add("--normalize")
+        .addExecPath("--sources", compressedApk)
+        .addExecPath("--output", outApk);
     setSingleJarAsExecutable(ruleContext, singleJarActionBuilder);
 
     if (javaResourceZip != null) {
@@ -306,64 +301,50 @@ public class ApkActionsBuilder {
               .setExecutable(resourceExtractor)
               .setMnemonic("ResourceExtractor")
               .setProgressMessage("Extracting Java resources from deploy jar for %s", apkName)
-              .addInputArgument(javaResourceZip)
-              .addOutputArgument(extractedJavaResourceZip)
+              .addInput(javaResourceZip)
+              .addOutput(extractedJavaResourceZip)
+              .setCommandLine(
+                  CustomCommandLine.builder()
+                      .addExecPath(javaResourceZip)
+                      .addExecPath(extractedJavaResourceZip)
+                      .build())
               .build(ruleContext));
 
       if (ruleContext.getFragment(AndroidConfiguration.class).compressJavaResources()) {
-        compressedApkActionBuilder
-            .addArgument("--sources")
-            .addInputArgument(extractedJavaResourceZip);
+        compressedApkActionBuilder.addInput(extractedJavaResourceZip);
+        compressedApkCommandLine.addExecPath("--sources", extractedJavaResourceZip);
       } else {
-        singleJarActionBuilder
-            .addArgument("--sources")
-            .addInputArgument(extractedJavaResourceZip);
+        singleJarActionBuilder.addInput(extractedJavaResourceZip);
+        singleJarCommandLine.addExecPath("--sources", extractedJavaResourceZip);
       }
     }
 
     if (nativeLibs.getName() != null) {
-      singleJarActionBuilder
-          .addArgument("--resources")
-          .addArgument(
-              singleJarResourcesArgument(
-                  nativeLibs.getName().getExecPathString(),
-                  nativeLibs.getName().getFilename()))
-          .addInput(nativeLibs.getName());
+      singleJarActionBuilder.addInput(nativeLibs.getName());
+      singleJarCommandLine
+          .add("--resources")
+          .addFormatted("%s:%s", nativeLibs.getName(), nativeLibs.getName().getFilename());
     }
 
     for (Artifact inputZip : inputZips.build()) {
-      singleJarActionBuilder.addArgument("--sources").addInputArgument(inputZip);
+      singleJarActionBuilder.addInput(inputZip);
+      singleJarCommandLine.addExecPath("--sources", inputZip);
     }
 
     ImmutableList<String> noCompressExtensions =
         ruleContext.getTokenizedStringListAttr("nocompress_extensions");
     if (ruleContext.getFragment(AndroidConfiguration.class).useNocompressExtensionsOnApk()
         && !noCompressExtensions.isEmpty()) {
-      compressedApkActionBuilder
-          .addArgument("--nocompress_suffixes")
-          .addArguments(noCompressExtensions);
-      singleJarActionBuilder
-          .addArgument("--nocompress_suffixes")
-          .addArguments(noCompressExtensions);
+      compressedApkCommandLine.addAll("--nocompress_suffixes", noCompressExtensions);
+      singleJarCommandLine.addAll("--nocompress_suffixes", noCompressExtensions);
     }
 
+    compressedApkActionBuilder.setCommandLine(compressedApkCommandLine.build());
     ruleContext.registerAction(compressedApkActionBuilder.build(ruleContext));
+    singleJarActionBuilder.setCommandLine(singleJarCommandLine.build());
     ruleContext.registerAction(singleJarActionBuilder.build(ruleContext));
   }
 
-  /**
-   * The --resources flag to singlejar can have either of the following forms:
-   * <ul>
-   * <li>The path to the input file. In this case the file is placed at the same path in the APK.
-   * <li>{@code from}:{@code to} where {@code from} is that path to the input file and {@code to} is
-   * the location in the APK to put it.
-   * </ul>
-   * This method creates the syntax for the second form.
-   */
-  private static String singleJarResourcesArgument(String from, String to) {
-    return from + ":" + to;
-  }
-
   /** Uses the zipalign tool to align the zip boundaries for uncompressed resources by 4 bytes. */
   private void zipalignApk(RuleContext ruleContext, Artifact inputApk, Artifact zipAlignedApk) {
     ruleContext.registerAction(
@@ -371,11 +352,16 @@ public class ApkActionsBuilder {
             .addInput(inputApk)
             .addOutput(zipAlignedApk)
             .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getZipalign())
-            .addArgument("4")
-            .addInputArgument(inputApk)
-            .addOutputArgument(zipAlignedApk)
             .setProgressMessage("Zipaligning %s", apkName)
             .setMnemonic("AndroidZipAlign")
+            .addInput(inputApk)
+            .addOutput(zipAlignedApk)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .add("4")
+                    .addExecPath(inputApk)
+                    .addExecPath(zipAlignedApk)
+                    .build())
             .build(ruleContext));
   }
 
@@ -393,16 +379,22 @@ public class ApkActionsBuilder {
             .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getApkSigner())
             .setProgressMessage("Signing %s", apkName)
             .setMnemonic("ApkSignerTool")
-            .addArgument("sign")
-            .addArgument("--ks")
-            .addInputArgument(signingKey)
-            .addArguments("--ks-pass", "pass:android")
-            .addArguments("--v1-signing-enabled", Boolean.toString(signingMethod.signV1()))
-            .addArguments("--v1-signer-name", "CERT")
-            .addArguments("--v2-signing-enabled", Boolean.toString(signingMethod.signV2()))
-            .addArgument("--out")
-            .addOutputArgument(signedAndZipalignedApk)
-            .addInputArgument(unsignedApk)
+            .addInput(signingKey)
+            .addOutput(signedAndZipalignedApk)
+            .addInput(unsignedApk)
+            .setCommandLine(
+                CustomCommandLine.builder()
+                    .add("sign")
+                    .add("--ks")
+                    .addExecPath(signingKey)
+                    .add("--ks-pass", "pass:android")
+                    .add("--v1-signing-enabled", Boolean.toString(signingMethod.signV1()))
+                    .add("--v1-signer-name", "CERT")
+                    .add("--v2-signing-enabled", Boolean.toString(signingMethod.signV2()))
+                    .add("--out")
+                    .addExecPath(signedAndZipalignedApk)
+                    .addExecPath(unsignedApk)
+                    .build())
             .build(ruleContext));
   }