Make sure to always use a parameter file when building Java deploy jars with SingleJar.

Otherwise the WorkerSpawnStrategy (which requires the action to use a parameter file) will fail to execute JavaDeployJar Spawn actions when its CommandLine decides not to use a parameter file, because the arguments are short enough.

Also adds a convenience method SpawnAction#getArgumentsFromParamFile() for use in tests, because I needed it here to fix up the deploy jar tests.

--
MOS_MIGRATED_REVID=132557693

5 files changed, 92 insertions, 55 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java
index 53adca39bd..c01afcf211 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis.actions;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
@@ -53,9 +54,19 @@ public abstract class CommandLine {
   }
 
   /**
-   * A default implementation of a command line backed by a copy of the given list of arguments.
+   * Returns the {@link ParameterFileWriteAction} that generates the parameter file used in this
+   * command line, or null if no parameter file is used.
    */
-  static CommandLine ofInternal(Iterable<String> arguments, final boolean isShellCommand) {
+  @VisibleForTesting
+  public ParameterFileWriteAction parameterFileWriteAction() {
+    return null;
+  }
+
+  /** A default implementation of a command line backed by a copy of the given list of arguments. */
+  static CommandLine ofInternal(
+      Iterable<String> arguments,
+      final boolean isShellCommand,
+      final ParameterFileWriteAction paramFileWriteAction) {
     final Iterable<String> immutableArguments = CollectionUtils.makeImmutable(arguments);
     return new CommandLine() {
       @Override
@@ -67,6 +78,11 @@ public abstract class CommandLine {
       public boolean isShellCommand() {
         return isShellCommand;
       }
+
+      @Override
+      public ParameterFileWriteAction parameterFileWriteAction() {
+        return paramFileWriteAction;
+      }
     };
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
index 20834f921d..f1c7c16174 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis.actions;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ParameterFile;
@@ -88,15 +87,17 @@ public final class ParamFileHelper {
    * @param isShellCommand true if this is a shell command
    * @param paramFileInfo parameter file information
    * @param parameterFile the output parameter file artifact
+   * @param paramFileWriteAction the action that generates the parameter file
    */
   public static CommandLine createWithParamsFile(
       List<String> executableArgs,
       boolean isShellCommand,
       ParamFileInfo paramFileInfo,
-      Artifact parameterFile) {
+      Artifact parameterFile,
+      ParameterFileWriteAction paramFileWriteAction) {
     String pathWithFlag = paramFileInfo.getFlag() + parameterFile.getExecPathString();
     Iterable<String> commandArgv = Iterables.concat(executableArgs, ImmutableList.of(pathWithFlag));
-    return CommandLine.ofInternal(commandArgv, isShellCommand);
+    return CommandLine.ofInternal(commandArgv, isShellCommand, paramFileWriteAction);
   }
 
   /**
@@ -104,19 +105,19 @@ public final class ParamFileHelper {
    *
    * @param arguments arguments to the command (in addition to executableArgs), OR
    * @param commandLine a {@link CommandLine} that provides the arguments (in addition to
-   *        executableArgs)
+   *     executableArgs)
    * @param owner owner of the action
    * @param parameterFile the output parameter file artifact
    * @param paramFileInfo parameter file information
    */
-  public static Action createParameterFileWriteAction(
+  public static ParameterFileWriteAction createParameterFileWriteAction(
       @Nullable Iterable<String> arguments,
       @Nullable CommandLine commandLine,
       ActionOwner owner,
       Artifact parameterFile,
       ParamFileInfo paramFileInfo) {
     CommandLine paramFileContents =
-        (commandLine != null) ? commandLine : CommandLine.ofInternal(arguments, false);
+        (commandLine != null) ? commandLine : CommandLine.ofInternal(arguments, false, null);
 
     return new ParameterFileWriteAction(owner, parameterFile, paramFileContents,
         paramFileInfo.getFileType(), paramFileInfo.getCharset());
@@ -137,7 +138,7 @@ public final class ParamFileHelper {
       Iterable<String> arguments, CommandLine commandLine, boolean isShellCommand) {
     if (commandLine == null) {
       Iterable<String> commandArgv = Iterables.concat(executableArgs, arguments);
-      return CommandLine.ofInternal(commandArgv, isShellCommand);
+      return CommandLine.ofInternal(commandArgv, isShellCommand, null);
     }
 
     if (executableArgs.isEmpty()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index 999f094430..3cb899a175 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -208,9 +208,19 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
   }
 
   /**
-   * Returns command argument, argv[0].
+   * Returns the list of options written to the parameter file. Don't use this method outside tests.
+   * The list is often huge, resulting in significant garbage collection overhead.
    */
   @VisibleForTesting
+  public List<String> getArgumentsFromParamFile() {
+    if (argv.parameterFileWriteAction() != null) {
+      return ImmutableList.copyOf(argv.parameterFileWriteAction().getContents());
+    }
+    return ImmutableList.of();
+  }
+
+  /** Returns command argument, argv[0]. */
+  @VisibleForTesting
   public String getCommandFilename() {
     return Iterables.getFirst(argv.arguments(), null);
   }
@@ -567,20 +577,24 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
           analysisEnvironment,
           outputs);
 
-      List<Action> actions = new ArrayList<>(2);
-
-      // Set up the SpawnAction itself.
-      actions.add(buildSpawnAction(owner, configuration.getLocalShellEnvironment(),
-          configuration.getShExecutable(), paramsFile));
-
       // If param file is to be used, set up the param file write action as well.
+      ParameterFileWriteAction paramFileWriteAction = null;
       if (paramsFile != null) {
-        actions.add(ParamFileHelper.createParameterFileWriteAction(
-            arguments,
-            commandLine,
-            owner,
-            paramsFile,
-            paramFileInfo));
+        paramFileWriteAction =
+            ParamFileHelper.createParameterFileWriteAction(
+                arguments, commandLine, owner, paramsFile, paramFileInfo);
+      }
+
+      List<Action> actions = new ArrayList<>(2);
+      actions.add(
+          buildSpawnAction(
+              owner,
+              configuration.getLocalShellEnvironment(),
+              configuration.getShExecutable(),
+              paramsFile,
+              paramFileWriteAction));
+      if (paramFileWriteAction != null) {
+        actions.add(paramFileWriteAction);
       }
 
       return actions.toArray(new Action[actions.size()]);
@@ -600,21 +614,25 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      * @param defaultShellEnvironment the default shell environment to use. May be null if not used.
      * @param defaultShellExecutable the default shell executable path. May be null if not used.
      * @param paramsFile the parameter file for the SpawnAction. May be null if not used.
+     * @param paramFileWriteAction the action generating the parameter file. May be null if not
+     *     used.
      * @return the SpawnAction and any actions required by it, with the first item always being the
-     *      SpawnAction itself.
+     *     SpawnAction itself.
      */
     SpawnAction buildSpawnAction(
         ActionOwner owner,
         @Nullable Map<String, String> defaultShellEnvironment,
         @Nullable PathFragment defaultShellExecutable,
-        @Nullable Artifact paramsFile) {
+        @Nullable Artifact paramsFile,
+        @Nullable ParameterFileWriteAction paramFileWriteAction) {
       List<String> argv = buildExecutableArgs(defaultShellExecutable);
       Iterable<String> arguments = argumentsBuilder.build();
       CommandLine actualCommandLine;
       if (paramsFile != null) {
         inputsBuilder.add(paramsFile);
-        actualCommandLine = ParamFileHelper.createWithParamsFile(argv, isShellCommand,
-            paramFileInfo, paramsFile);
+        actualCommandLine =
+            ParamFileHelper.createWithParamsFile(
+                argv, isShellCommand, paramFileInfo, paramsFile, paramFileWriteAction);
       } else {
         actualCommandLine = ParamFileHelper.createWithoutParamsFile(argv, arguments, commandLine,
             isShellCommand);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
index 0d72686cc1..65724891bc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis.actions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Actions;
@@ -167,7 +166,8 @@ public final class SpawnActionTemplate implements ActionAnalysisMetadata {
         getOwner(),
         /*defaultShellEnvironment=*/ null,
         /*defaultShellExecutable=*/ null,
-        /*paramsFile=*/ null);
+        /*paramsFile=*/ null,
+        /*paramFileWriteAction=*/ null);
   }
 
   private static void checkActionAndArtifactConflicts(Iterable<ActionAnalysisMetadata> actions)
@@ -266,7 +266,7 @@ public final class SpawnActionTemplate implements ActionAnalysisMetadata {
   @Override
   public Iterable<String> getClientEnvironmentVariables() {
     return spawnActionBuilder
-        .buildSpawnAction(getOwner(), null, null, null)
+        .buildSpawnAction(getOwner(), null, null, null, null)
         .getClientEnvironmentVariables();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java
index edff8492e2..2a72321d61 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java
@@ -239,33 +239,35 @@ public class DeployArchiveBuilder {
     // eliminate this check, allowing only native singlejar.
     Artifact singlejar = getSingleJar(ruleContext);
     if (singlejar.getFilename().endsWith(".jar")) {
-      ruleContext.registerAction(new SpawnAction.Builder()
-          .addInputs(inputs.build())
-          .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext))
-          .addOutput(outputJar)
-          .setResources(resourceSet)
-          .setJarExecutable(
-              ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(),
-              singlejar,
-              jvmArgs)
-          .setCommandLine(commandLine)
-          .useParameterFile(ParameterFileType.SHELL_QUOTED)
-          .setProgressMessage("Building deploy jar " + outputJar.prettyPrint())
-          .setMnemonic("JavaDeployJar")
-          .setExecutionInfo(ImmutableMap.of("supports-workers", "1"))
-          .build(ruleContext));
+      ruleContext.registerAction(
+          new SpawnAction.Builder()
+              .addInputs(inputs.build())
+              .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext))
+              .addOutput(outputJar)
+              .setResources(resourceSet)
+              .setJarExecutable(
+                  ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(),
+                  singlejar,
+                  jvmArgs)
+              .setCommandLine(commandLine)
+              .alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED)
+              .setProgressMessage("Building deploy jar " + outputJar.prettyPrint())
+              .setMnemonic("JavaDeployJar")
+              .setExecutionInfo(ImmutableMap.of("supports-workers", "1"))
+              .build(ruleContext));
     } else {
-      ruleContext.registerAction(new SpawnAction.Builder()
-          .addInputs(inputs.build())
-          .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext))
-          .addOutput(outputJar)
-          .setResources(resourceSet)
-          .setExecutable(singlejar)
-          .setCommandLine(commandLine)
-          .useParameterFile(ParameterFileType.SHELL_QUOTED)
-          .setProgressMessage("Building deploy jar " + outputJar.prettyPrint())
-          .setMnemonic("JavaDeployJar")
-          .build(ruleContext));
+      ruleContext.registerAction(
+          new SpawnAction.Builder()
+              .addInputs(inputs.build())
+              .addTransitiveInputs(JavaHelper.getHostJavabaseInputs(ruleContext))
+              .addOutput(outputJar)
+              .setResources(resourceSet)
+              .setExecutable(singlejar)
+              .setCommandLine(commandLine)
+              .alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED)
+              .setProgressMessage("Building deploy jar " + outputJar.prettyPrint())
+              .setMnemonic("JavaDeployJar")
+              .build(ruleContext));
     }
   }