Automated rollback of commit 4a2e51b.

*** Reason for rollback ***

Breaks targets on the nightly TGP.

Reproduction:

blaze build //third_party/bazel_rules/rules_kotlin/tests/android/java/com/google/jni:AndroidJniTest --compilation_mode=opt --flaky_test_attempts=2 --fat_apk_cpu=x86 --android_platforms=//buildenv/platforms/android:x86 --incompatible_enable_android_toolchain_resolution=1 --collect_code_coverage=1 --instrumentation_filter=//java/com/google/android/samples/apps/topeka[/:],//third_party/bazel_rules/rules_kotlin[/:],//tools/build_defs/kotlin[/:]

TGP link: []

*** Original change description ***

Make `getPrimaryOutput()` always return the first artifact in `getOutputs()`.

This is already the case everywhere except `CppCompileAction` but was not documented, leading to `SpawnAction` unnecessarily storing a field for the primary output when it's already just the first element in its outputs. This change saves 8 bytes per `SpawnAction` and `GenRuleAction`, and moves other `SpawnAction` subclasses closer to an 8-byte threshold.

`CppCompileAction` had been using the coverage artifact (if pr

***

RELNOTES: None.
PiperOrigin-RevId: 523634597
Change-Id: I0aa70851fe4d403afabc56e808546d6638a9f2b7

Author: lberki

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

@@ -309,8 +309,10 @@ public Artifact getPrimaryInput() {
   }
 
   @Override
-  public final Artifact getPrimaryOutput() {
-    return Iterables.get(outputs, 0);
+  public Artifact getPrimaryOutput() {
+    // Default behavior is to return the first output artifact.
+    // Use the method rather than field in case of overriding in subclasses.
+    return Iterables.getFirst(getOutputs(), null);
   }
 
   @Override

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

@@ -186,8 +186,7 @@ NestedSet<Artifact> getInputFilesForExtraAction(ActionExecutionContext actionExe
   Artifact getPrimaryInput();
 
   /**
-   * Returns the "primary" output of this action, which is the same as the first artifact in {@link
-   * #getOutputs}.
+   * Returns the "primary" output of this action.
    *
    * <p>For example, the linked library would be the primary output of a LinkAction.
    *

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

@@ -48,9 +48,9 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) {
   }
 
   /**
-   * Returns a human-readable description of the inputs to {@link #getKey}. Used in the output from
-   * '--explain', and in error messages for '--check_up_to_date' and '--check_tests_up_to_date'. May
-   * return null, meaning no extra information is available.
+   * Returns a human-readable description of the inputs to {@link #getKey(ActionKeyContext)}. Used
+   * in the output from '--explain', and in error messages for '--check_up_to_date' and
+   * '--check_tests_up_to_date'. May return null, meaning no extra information is available.
    *
    * <p>If the return value is non-null, for consistency it should be a multiline message of the
    * form:

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

@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.actions;
 
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
@@ -104,7 +103,7 @@ public interface ExtraActionInfoSupplier {
 
   private static final String GUID = "ebd6fce3-093e-45ee-adb6-bf513b602f0d";
 
-  private static final Interner<ImmutableSortedMap<String, String>> executionInfoInterner =
+  public static final Interner<ImmutableSortedMap<String, String>> executionInfoInterner =
       BlazeInterners.newWeakInterner();
 
   private final CommandLines commandLines;
@@ -119,6 +118,7 @@ public interface ExtraActionInfoSupplier {
   private final ImmutableMap<String, String> executionInfo;
 
   private final ExtraActionInfoSupplier extraActionInfoSupplier;
+  private final Artifact primaryOutput;
   private final Consumer<Pair<ActionExecutionContext, List<SpawnResult>>> resultConsumer;
   private final boolean stripOutputPaths;
 
@@ -132,6 +132,7 @@ public interface ExtraActionInfoSupplier {
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified.
    * @param outputs the set of all files written by this action; must not be subsequently modified.
+   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action.
    * @param env the action environment
    * @param commandLines the command lines to execute. This includes the main argv vector and any
@@ -147,6 +148,7 @@ public SpawnAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<Artifact> outputs,
+      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -159,6 +161,7 @@ public SpawnAction(
         tools,
         inputs,
         outputs,
+        primaryOutput,
         resourceSetOrBuilder,
         commandLines,
         commandLineLimits,
@@ -185,6 +188,7 @@ public SpawnAction(
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified
    * @param outputs the set of all files written by this action; must not be subsequently modified.
+   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action.
    * @param env the action's environment
    * @param executionInfo out-of-band information for scheduling the spawn
@@ -202,6 +206,7 @@ public SpawnAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<? extends Artifact> outputs,
+      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -216,6 +221,7 @@ public SpawnAction(
       Consumer<Pair<ActionExecutionContext, List<SpawnResult>>> resultConsumer,
       boolean stripOutputPaths) {
     super(owner, tools, inputs, runfilesSupplier, outputs, env);
+    this.primaryOutput = primaryOutput;
     this.resourceSetOrBuilder = resourceSetOrBuilder;
     this.executionInfo =
         executionInfo.isEmpty()
@@ -232,6 +238,11 @@ public SpawnAction(
     this.stripOutputPaths = stripOutputPaths;
   }
 
+  @Override
+  public Artifact getPrimaryOutput() {
+    return primaryOutput;
+  }
+
   @VisibleForTesting
   public CommandLines getCommandLines() {
     return commandLines;
@@ -250,10 +261,12 @@ public List<String> getArguments() throws CommandLineExpansionException, Interru
   }
 
   @Override
-  public Sequence<CommandLineArgsApi> getStarlarkArgs() {
+  public Sequence<CommandLineArgsApi> getStarlarkArgs() throws EvalException {
     ImmutableList.Builder<CommandLineArgsApi> result = ImmutableList.builder();
     ImmutableSet<Artifact> directoryInputs =
-        getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet());
+        getInputs().toList().stream()
+            .filter(artifact -> artifact.isDirectory())
+            .collect(ImmutableSet.toImmutableSet());
 
     for (CommandLineAndParamFileInfo commandLine : commandLines.getCommandLines()) {
       result.add(Args.forRegisteredAction(commandLine, directoryInputs));
@@ -523,7 +536,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont
    * <p>Subclasses of SpawnAction may override this in order to provide action-specific behaviour.
    * This can be necessary, for example, when the action discovers inputs.
    */
-  private SpawnInfo getExtraActionSpawnInfo()
+  protected SpawnInfo getExtraActionSpawnInfo()
       throws CommandLineExpansionException, InterruptedException {
     SpawnInfo.Builder info = SpawnInfo.newBuilder();
     Spawn spawn = getSpawnForExtraAction();
@@ -588,7 +601,7 @@ private ActionSpawn(
         throws CommandLineExpansionException {
       super(
           arguments,
-          ImmutableMap.of(),
+          ImmutableMap.<String, String>of(),
           parent.getExecutionInfo(),
           parent.getRunfilesSupplier(),
           parent,
@@ -792,7 +805,8 @@ private SpawnAction buildSpawnAction(
           owner,
           tools,
           inputsAndTools,
-          ImmutableSet.copyOf(outputs),
+          ImmutableList.copyOf(outputs),
+          outputs.get(0),
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -812,7 +826,8 @@ protected SpawnAction createSpawnAction(
         ActionOwner owner,
         NestedSet<Artifact> tools,
         NestedSet<Artifact> inputsAndTools,
-        ImmutableSet<Artifact> outputs,
+        ImmutableList<Artifact> outputs,
+        Artifact primaryOutput,
         ResourceSetOrBuilder resourceSetOrBuilder,
         CommandLines commandLines,
         CommandLineLimits commandLineLimits,
@@ -828,6 +843,7 @@ protected SpawnAction createSpawnAction(
           tools,
           inputsAndTools,
           outputs,
+          primaryOutput,
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -933,6 +949,13 @@ public Builder addOutputs(Iterable<Artifact> artifacts) {
       return this;
     }
 
+    /**
+     * Checks whether the action produces any outputs
+     */
+    public boolean hasOutputs() {
+      return !outputs.isEmpty();
+    }
+
     /**
      * Sets RecourceSet for builder. If ResourceSetBuilder set, then ResourceSetBuilder will
      * override setResources.

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

@@ -18,8 +18,6 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Action;
@@ -83,6 +81,7 @@ public final class StarlarkAction extends SpawnAction implements ActionCacheAwar
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified
    * @param outputs the set of all files written by this action; must not be subsequently modified.
+   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action
    * @param commandLines the command lines to execute. This includes the main argv vector and any
    *     param file-backed command lines.
@@ -104,6 +103,7 @@ private StarlarkAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<Artifact> outputs,
+      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -123,6 +123,7 @@ private StarlarkAction(
             ? createInputs(shadowedAction.get().getInputs(), inputs)
             : inputs,
         outputs,
+        primaryOutput,
         resourceSetOrBuilder,
         commandLines,
         commandLineLimits,
@@ -375,7 +376,8 @@ protected SpawnAction createSpawnAction(
         ActionOwner owner,
         NestedSet<Artifact> tools,
         NestedSet<Artifact> inputsAndTools,
-        ImmutableSet<Artifact> outputs,
+        ImmutableList<Artifact> outputs,
+        Artifact primaryOutput,
         ResourceSetOrBuilder resourceSetOrBuilder,
         CommandLines commandLines,
         CommandLineLimits commandLineLimits,
@@ -401,6 +403,7 @@ protected SpawnAction createSpawnAction(
           tools,
           inputsAndTools,
           outputs,
+          primaryOutput,
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -412,7 +415,7 @@ protected SpawnAction createSpawnAction(
           mnemonic,
           unusedInputsList,
           shadowedAction,
-          stripOutputPaths(mnemonic, inputsAndTools, outputs, configuration));
+          stripOutputPaths(mnemonic, inputsAndTools, primaryOutput, configuration));
     }
 
     /**
@@ -432,7 +435,7 @@ protected SpawnAction createSpawnAction(
     private static boolean stripOutputPaths(
         String mnemonic,
         NestedSet<Artifact> inputs,
-        ImmutableSet<Artifact> outputs,
+        Artifact primaryOutput,
         BuildConfigurationValue configuration) {
       ImmutableList<String> qualifyingMnemonics =
           ImmutableList.of(
@@ -453,8 +456,7 @@ private static boolean stripOutputPaths(
       CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class);
       return coreOptions.outputPathsMode == OutputPathsMode.STRIP
           && qualifyingMnemonics.contains(mnemonic)
-          && PathStripper.isPathStrippable(
-              inputs, Iterables.get(outputs, 0).getExecPath().subFragment(0, 1));
+          && PathStripper.isPathStrippable(inputs, primaryOutput.getExecPath().subFragment(0, 1));
     }
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java

@@ -14,8 +14,10 @@
 
 package com.google.devtools.build.lib.analysis.extra;
 
+import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.Action;
@@ -54,6 +56,9 @@ public final class ExtraAction extends SpawnAction {
   private final boolean createDummyOutput;
   private final NestedSet<Artifact> extraActionInputs;
 
+  public static final Function<ExtraAction, Action> GET_SHADOWED_ACTION =
+      e -> e != null ? e.getShadowedAction() : null;
+
   ExtraAction(
       NestedSet<Artifact> extraActionInputs,
       RunfilesSupplier runfilesSupplier,
@@ -73,6 +78,7 @@ public final class ExtraAction extends SpawnAction {
             NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             extraActionInputs),
         outputs,
+        Iterables.getFirst(outputs, null),
         AbstractAction.DEFAULT_RESOURCE_SET,
         CommandLines.of(argv),
         CommandLineLimits.UNLIMITED,
@@ -122,7 +128,7 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
     updateInputs(
         createInputs(shadowedAction.getInputs(), inputFilesForExtraAction, extraActionInputs));
     return NestedSetBuilder.wrap(
-        Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet()));
+        Order.STABLE_ORDER, Sets.<Artifact>difference(getInputs().toSet(), oldInputs.toSet()));
   }
 
   private static NestedSet<Artifact> createInputs(

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -676,7 +676,7 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
       env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1));
     }
     // TODO(b/184206260): Actually set TEST_RANDOM_SEED with random seed.
-    // The above TEST_RANDOM_SEED has historically been set with the run number, but we should
+    // The above TEST_RANDOM_SEED has histroically been set with the run number, but we should
     // explicitly set TEST_RUN_NUMBER to indicate the run number and actually set TEST_RANDOM_SEED
     // with a random seed. However, much code has come to depend on it being set to the run number
     // and this is an externally documented behavior. Modifying TEST_RANDOM_SEED should be done
@@ -923,6 +923,11 @@ public String getRunfilesPrefix() {
     return workspaceName;
   }
 
+  @Override
+  public Artifact getPrimaryOutput() {
+    return testLog;
+  }
+
   public PackageSpecificationProvider getNetworkAllowlist() {
     return networkAllowlist;
   }

src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java

@@ -1732,7 +1732,7 @@ private void createModuleCodegenAction(
         configuration, featureConfiguration, builder, ruleErrorConsumer);
     CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(compileAction);
-    Artifact objectFile = compileAction.getPrimaryOutput();
+    Artifact objectFile = compileAction.getOutputFile();
     if (pic) {
       result.addPicObjectFile(objectFile);
     } else {
@@ -1775,7 +1775,7 @@ private void createHeaderAction(
         configuration, featureConfiguration, builder, ruleErrorConsumer);
     CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(compileAction);
-    Artifact tokenFile = compileAction.getPrimaryOutput();
+    Artifact tokenFile = compileAction.getOutputFile();
     result.addHeaderTokenFile(tokenFile);
   }
 
@@ -1868,13 +1868,13 @@ private ImmutableList<Artifact> createSourceAction(
           configuration, featureConfiguration, picBuilder, ruleErrorConsumer);
       CppCompileAction picAction = picBuilder.buildOrThrowRuleError(ruleErrorConsumer);
       actionRegistry.registerAction(picAction);
-      directOutputs.add(picAction.getPrimaryOutput());
+      directOutputs.add(picAction.getOutputFile());
       if (addObject) {
-        result.addPicObjectFile(picAction.getPrimaryOutput());
+        result.addPicObjectFile(picAction.getOutputFile());
 
         if (bitcodeOutput) {
           result.addLtoBitcodeFile(
-              picAction.getPrimaryOutput(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
+              picAction.getOutputFile(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
         }
       }
       if (dwoFile != null) {
@@ -1939,7 +1939,7 @@ private ImmutableList<Artifact> createSourceAction(
           configuration, featureConfiguration, builder, ruleErrorConsumer);
       CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
       actionRegistry.registerAction(compileAction);
-      Artifact objectFile = compileAction.getPrimaryOutput();
+      Artifact objectFile = compileAction.getOutputFile();
       directOutputs.add(objectFile);
       if (addObject) {
         result.addObjectFile(objectFile);
@@ -2118,7 +2118,7 @@ private ImmutableList<Artifact> createTempsActions(
     CppCompileAction sdAction = sdBuilder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(sdAction);
 
-    return ImmutableList.of(dAction.getPrimaryOutput(), sdAction.getPrimaryOutput());
+    return ImmutableList.of(dAction.getOutputFile(), sdAction.getOutputFile());
   }
 
   /**