Remove CommandLineLimits field from SpawnAction.

In most cases, it comes from the `BuildConfigurationValue`, which after recent changes is now accessible via the `ActionOwner`. In some cases, it is overridden to be `CommandLineLimits.UNLIMITED`, in which case subclasses can override `getCommandLineLimits()`.

`SpawnActionTemplate` now uses a subclass since it uses `UNLIMITED`. `SpawnActionTest` is updated to use an `ActionOwner` with an accurate `BuildConfigurationValue`.

PiperOrigin-RevId: 527883149
Change-Id: I6406941540155f2ea1cb3af072fec42e92828280

src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java

@@ -108,14 +108,8 @@ public static ActionOwner createDummy(
   /** Returns the target kind (rule class name) for this {@link ActionOwner}. */
   public abstract String getTargetKind();
 
-  /**
-   * Returns {@link BuildConfigurationInfo} for this {@link ActionOwner}.
-   *
-   * <p>This method is kept solely for AutoValue class purpose, so call {@link #getMnemonic()},
-   * {@link #getConfigurationChecksum()}, {@link #getBuildConfigurationEvent()} and {@link
-   * #isBuildConfigurationForTool()} to access its fields instead.
-   */
-  abstract BuildConfigurationInfo getBuildConfigurationInfo();
+  /** Returns {@link BuildConfigurationInfo} for this {@link ActionOwner}. */
+  public abstract BuildConfigurationInfo getBuildConfigurationInfo();
 
   /** Returns the mnemonic for the configuration for this {@link ActionOwner}. */
   public final String getMnemonic() {

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1499,10 +1499,12 @@ java_library(
     deps = [
         ":actions/custom_command_line",
         ":analysis_cluster",
+        ":config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
+        "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
         "//src/main/java/com/google/devtools/build/lib/actions:middleman_type",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_template_expansion_value",

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

@@ -104,7 +104,6 @@ public class SpawnAction extends AbstractAction implements CommandAction {
       BlazeInterners.newWeakInterner();
 
   private final CommandLines commandLines;
-  private final CommandLineLimits commandLineLimits;
 
   private final CharSequence progressMessage;
   private final String mnemonic;
@@ -127,7 +126,6 @@ public class SpawnAction extends AbstractAction implements CommandAction {
    * @param env the action environment
    * @param commandLines the command lines to execute. This includes the main argv vector and any
    *     param file-backed command lines.
-   * @param commandLineLimits the command line limits, from the build configuration
    * @param progressMessage the message printed during the progression of the build.
    * @param mnemonic the mnemonic that is reported in the master log.
    */
@@ -138,7 +136,6 @@ public SpawnAction(
       Iterable<Artifact> outputs,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
-      CommandLineLimits commandLineLimits,
       ActionEnvironment env,
       CharSequence progressMessage,
       String mnemonic) {
@@ -149,7 +146,6 @@ public SpawnAction(
         outputs,
         resourceSetOrBuilder,
         commandLines,
-        commandLineLimits,
         env,
         ImmutableMap.of(),
         progressMessage,
@@ -174,7 +170,6 @@ public SpawnAction(
    * @param executionInfo out-of-band information for scheduling the spawn
    * @param commandLines the command lines to execute. This includes the main argv vector and any
    *     param file-backed command lines.
-   * @param commandLineLimits the command line limits, from the build configuration
    * @param progressMessage the message printed during the progression of the build
    * @param runfilesSupplier {@link RunfilesSupplier}s describing the runfiles for the action
    * @param mnemonic the mnemonic that is reported in the master log
@@ -186,7 +181,6 @@ public SpawnAction(
       Iterable<? extends Artifact> outputs,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
-      CommandLineLimits commandLineLimits,
       ActionEnvironment env,
       ImmutableMap<String, String> executionInfo,
       CharSequence progressMessage,
@@ -200,7 +194,6 @@ public SpawnAction(
             ? ImmutableSortedMap.of()
             : executionInfoInterner.intern(ImmutableSortedMap.copyOf(executionInfo));
     this.commandLines = commandLines;
-    this.commandLineLimits = commandLineLimits;
     this.progressMessage = progressMessage;
     this.mnemonic = mnemonic;
     this.stripOutputPaths = stripOutputPaths;
@@ -378,7 +371,7 @@ protected Spawn getSpawn(
             artifactExpander,
             getPrimaryOutput().getExecPath(),
             getPathStripper(),
-            commandLineLimits);
+            getCommandLineLimits());
     return new ActionSpawn(
         ImmutableList.copyOf(expandedCommandLines.arguments()),
         this,
@@ -391,6 +384,11 @@ protected Spawn getSpawn(
         stripOutputPaths);
   }
 
+  @ForOverride
+  protected CommandLineLimits getCommandLineLimits() {
+    return getOwner().getBuildConfigurationInfo().getCommandLineLimits();
+  }
+
   Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException {
     return getSpawn(getInputs());
   }
@@ -623,15 +621,11 @@ public static class Builder {
     private String execGroup = DEFAULT_EXEC_GROUP_NAME;
     private boolean stripOutputPaths = false;
 
-    /**
-     * Creates a SpawnAction builder.
-     */
+    /** Creates a SpawnAction builder. */
     public Builder() {}
 
-    /**
-     * Creates a builder that is a copy of another builder.
-     */
-    public Builder(Builder other) {
+    /** Creates a builder that is a copy of another builder. */
+    Builder(Builder other) {
       this.toolsBuilder.addTransitive(other.toolsBuilder.build());
       this.inputsBuilder.addTransitive(other.inputsBuilder.build());
       this.outputs.addAll(other.outputs);
@@ -687,8 +681,7 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio
               : useDefaultShellEnvironment
                   ? configuration.getActionEnvironment()
                   : ActionEnvironment.create(environment, inheritedEnvironment);
-      return buildSpawnAction(
-          owner, commandLines, configuration.getCommandLineLimits(), configuration, env);
+      return buildSpawnAction(owner, commandLines, configuration, env);
     }
 
     @CheckReturnValue
@@ -705,7 +698,6 @@ SpawnAction buildForActionTemplate(ActionOwner owner) {
       return buildSpawnAction(
           owner,
           result.build(),
-          CommandLineLimits.UNLIMITED,
           null,
           ActionEnvironment.create(environment, inheritedEnvironment));
     }
@@ -719,7 +711,6 @@ SpawnAction buildForActionTemplate(ActionOwner owner) {
     private SpawnAction buildSpawnAction(
         ActionOwner owner,
         CommandLines commandLines,
-        CommandLineLimits commandLineLimits,
         @Nullable BuildConfigurationValue configuration,
         ActionEnvironment env) {
       NestedSet<Artifact> tools = toolsBuilder.build();
@@ -745,7 +736,6 @@ private SpawnAction buildSpawnAction(
           ImmutableSet.copyOf(outputs),
           resourceSetOrBuilder,
           commandLines,
-          commandLineLimits,
           env,
           configuration,
           configuration == null
@@ -764,7 +754,6 @@ protected SpawnAction createSpawnAction(
         ImmutableSet<Artifact> outputs,
         ResourceSetOrBuilder resourceSetOrBuilder,
         CommandLines commandLines,
-        CommandLineLimits commandLineLimits,
         ActionEnvironment env,
         @Nullable BuildConfigurationValue configuration,
         ImmutableMap<String, String> executionInfo,
@@ -778,7 +767,6 @@ protected SpawnAction createSpawnAction(
           outputs,
           resourceSetOrBuilder,
           commandLines,
-          commandLineLimits,
           env,
           executionInfo,
           progressMessage,

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java

@@ -17,8 +17,10 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionKeyCacher;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -31,8 +33,13 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
+import com.google.devtools.build.lib.actions.CommandLineLimits;
+import com.google.devtools.build.lib.actions.CommandLines;
 import com.google.devtools.build.lib.actions.MiddlemanType;
+import com.google.devtools.build.lib.actions.ResourceSetOrBuilder;
+import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 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;
@@ -49,7 +56,6 @@ public final class SpawnActionTemplate extends ActionKeyCacher
     implements ActionTemplate<SpawnAction> {
   private final SpecialArtifact inputTreeArtifact;
   private final SpecialArtifact outputTreeArtifact;
-  private final NestedSet<Artifact> commonInputs;
   private final NestedSet<Artifact> allInputs;
   private final NestedSet<Artifact> commonTools;
   private final ActionOwner actionOwner;
@@ -89,14 +95,12 @@ private SpawnActionTemplate(
     this.inputTreeArtifact = inputTreeArtifact;
     this.outputTreeArtifact = outputTreeArtifact;
     this.commonTools = commonTools;
-    this.commonInputs = NestedSetBuilder.<Artifact>stableOrder()
-        .addTransitive(commonInputs)
-        .addTransitive(commonTools)
-        .build();
-    this.allInputs = NestedSetBuilder.<Artifact>stableOrder()
-        .add(inputTreeArtifact)
-        .addTransitive(this.commonInputs)
-        .build();
+    this.allInputs =
+        NestedSetBuilder.<Artifact>stableOrder()
+            .add(inputTreeArtifact)
+            .addTransitive(commonInputs)
+            .addTransitive(commonTools)
+            .build();
     this.outputPathMapper = outputPathMapper;
     this.actionOwner = actionOwner;
     this.mnemonic = mnemonic;
@@ -147,7 +151,7 @@ protected void computeKey(
    */
   private SpawnAction createAction(
       TreeFileArtifact inputTreeFileArtifact, TreeFileArtifact outputTreeFileArtifact) {
-    SpawnAction.Builder actionBuilder = new SpawnAction.Builder(spawnActionBuilder);
+    SpawnAction.Builder actionBuilder = new ExpandedSpawnAction.Builder(spawnActionBuilder);
     actionBuilder.addInput(inputTreeFileArtifact);
     actionBuilder.addOutput(outputTreeFileArtifact);
 
@@ -158,7 +162,7 @@ private SpawnAction createAction(
     // Note that we pass in nulls below because SpawnActionTemplate does not support param file, and
     // it does not use any default value for executable or shell environment. They must be set
     // explicitly via builder method #setExecutable and #setEnvironment.
-    return actionBuilder.buildForActionTemplate(getOwner());
+    return actionBuilder.buildForActionTemplate(actionOwner);
   }
 
   /**
@@ -189,7 +193,7 @@ public boolean isShareable() {
   }
 
   @Override
-  public final String getMnemonic() {
+  public String getMnemonic() {
     return mnemonic;
   }
 
@@ -210,7 +214,7 @@ public NestedSet<Artifact> getSchedulingDependencies() {
 
   @Override
   public NestedSet<Artifact> getMandatoryInputs() {
-    return getInputs();
+    return allInputs;
   }
 
   @Override
@@ -226,7 +230,7 @@ public ImmutableSet<Artifact> getMandatoryOutputs() {
 
   @Override
   public Collection<String> getClientEnvironmentVariables() {
-    return spawnActionBuilder.buildForActionTemplate(getOwner()).getClientEnvironmentVariables();
+    return spawnActionBuilder.buildForActionTemplate(actionOwner).getClientEnvironmentVariables();
   }
 
   @Override
@@ -327,17 +331,6 @@ public Builder addCommonInputs(Iterable<Artifact> artifacts) {
       return this;
     }
 
-    /**
-     * Adds transitive common input artifacts. All common input artifacts will be added as input
-     * artifacts for expanded actions.
-     */
-    @CanIgnoreReturnValue
-    public Builder addCommonTransitiveInputs(NestedSet<Artifact> artifacts) {
-      inputsBuilder.addTransitive(artifacts);
-      spawnActionBuilder.addTransitiveInputs(artifacts);
-      return this;
-    }
-
     /** Sets the map of environment variables for expanded actions. */
     @CanIgnoreReturnValue
     @Deprecated // TODO(ulfjack): Add env variables to the common environment, rather than replacing
@@ -441,4 +434,72 @@ public SpawnActionTemplate build(ActionOwner actionOwner) {
           spawnActionBuilder);
     }
   }
+
+  private static final class ExpandedSpawnAction extends SpawnAction {
+    ExpandedSpawnAction(
+        ActionOwner owner,
+        NestedSet<Artifact> tools,
+        NestedSet<Artifact> inputs,
+        Iterable<? extends Artifact> outputs,
+        ResourceSetOrBuilder resourceSetOrBuilder,
+        CommandLines commandLines,
+        ActionEnvironment env,
+        ImmutableMap<String, String> executionInfo,
+        CharSequence progressMessage,
+        RunfilesSupplier runfilesSupplier,
+        String mnemonic) {
+      super(
+          owner,
+          tools,
+          inputs,
+          outputs,
+          resourceSetOrBuilder,
+          commandLines,
+          env,
+          executionInfo,
+          progressMessage,
+          runfilesSupplier,
+          mnemonic,
+          /* stripOutputPaths= */ false);
+    }
+
+    @Override
+    protected CommandLineLimits getCommandLineLimits() {
+      return CommandLineLimits.UNLIMITED;
+    }
+
+    private static final class Builder extends SpawnAction.Builder {
+      Builder(SpawnAction.Builder template) {
+        super(template);
+      }
+
+      @Override
+      protected SpawnAction createSpawnAction(
+          ActionOwner owner,
+          NestedSet<Artifact> tools,
+          NestedSet<Artifact> inputsAndTools,
+          ImmutableSet<Artifact> outputs,
+          ResourceSetOrBuilder resourceSetOrBuilder,
+          CommandLines commandLines,
+          ActionEnvironment env,
+          @Nullable BuildConfigurationValue configuration,
+          ImmutableMap<String, String> executionInfo,
+          CharSequence progressMessage,
+          RunfilesSupplier runfilesSupplier,
+          String mnemonic) {
+        return new ExpandedSpawnAction(
+            owner,
+            tools,
+            inputsAndTools,
+            outputs,
+            resourceSetOrBuilder,
+            commandLines,
+            env,
+            executionInfo,
+            progressMessage,
+            runfilesSupplier,
+            mnemonic);
+      }
+    }
+  }
 }