Make test actions and other unshareable actions have an unshareable key.

Adds support for running tests to BlazeRuntimeWrapper.

PiperOrigin-RevId: 322441843

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

@@ -38,7 +38,17 @@ private ActionLookupData(ActionLookupKey actionLookupKey, int actionIndex) {
    * only be called once per {@code (actionLookupKey, actionIndex)} pair.
    */
   public static ActionLookupData create(ActionLookupKey actionLookupKey, int actionIndex) {
-    return new ActionLookupData(actionLookupKey, actionIndex);
+    // If the label is null, this is not a typical action (it may be the build info action or a
+    // coverage action, for example). Don't try to share it.
+    return actionLookupKey.getLabel() == null
+        ? createUnshareable(actionLookupKey, actionIndex)
+        : new ActionLookupData(actionLookupKey, actionIndex);
+  }
+
+  /** Similar to {@link #create}, but the key will have {@link ShareabilityOfValue#NEVER}. */
+  public static ActionLookupData createUnshareable(
+      ActionLookupKey actionLookupKey, int actionIndex) {
+    return new UnshareableActionLookupData(actionLookupKey, actionIndex);
   }
 
   public ActionLookupKey getActionLookupKey() {
@@ -57,12 +67,6 @@ public Label getLabel() {
     return actionLookupKey.getLabel();
   }
 
-  @Override
-  public ShareabilityOfValue getShareabilityOfValue() {
-    // If the label is null, this is a weird action. Don't try to share it.
-    return getLabel() != null ? SkyKey.super.getShareabilityOfValue() : ShareabilityOfValue.NEVER;
-  }
-
   @Override
   public int hashCode() {
     return 37 * actionLookupKey.hashCode() + actionIndex;
@@ -93,4 +97,15 @@ public String toString() {
   public SkyFunctionName functionName() {
     return SkyFunctions.ACTION_EXECUTION;
   }
+
+  private static final class UnshareableActionLookupData extends ActionLookupData {
+    private UnshareableActionLookupData(ActionLookupKey actionLookupKey, int actionIndex) {
+      super(actionLookupKey, actionIndex);
+    }
+
+    @Override
+    public ShareabilityOfValue getShareabilityOfValue() {
+      return ShareabilityOfValue.NEVER;
+    }
+  }
 }

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

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.actions;
 
+import static java.util.concurrent.TimeUnit.MINUTES;
+
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -23,8 +25,8 @@
 import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.packages.OutputFile;
+import com.google.devtools.build.lib.skyframe.SkyframeAwareAction;
 import com.google.devtools.build.lib.vfs.OsPathPolicy;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
@@ -35,15 +37,9 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.SortedMap;
-import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 
-/**
- * Helper class for actions.
- *
- * <p>We expect the class to be created exactly once per build doing shared actions check.
- */
-@ThreadSafe
+/** Utility class for actions. */
 public final class Actions {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
@@ -54,6 +50,28 @@ public final class Actions {
       .addEscape(':', "_C")
       .build();
 
+  private Actions() {}
+
+  /**
+   * Determines whether the given action needs to depend on the build ID.
+   *
+   * <p>Such actions are not shareable across servers.
+   */
+  public static boolean dependsOnBuildId(ActionAnalysisMetadata action) {
+    // Volatile build actions may need to execute even if none of their known inputs have changed.
+    // Depending on the build ID ensures that these actions have a chance to execute.
+    // SkyframeAwareActions do not need to depend on the build ID because their volatility is due to
+    // their dependence on Skyframe nodes that are not captured in the action cache. Any changes to
+    // those nodes will cause this action to be rerun, so a build ID dependency is unnecessary.
+    if (!(action instanceof Action)) {
+      return false;
+    }
+    if (action instanceof NotifyOnActionCacheHit) {
+      return true;
+    }
+    return ((Action) action).isVolatile() && !(action instanceof SkyframeAwareAction);
+  }
+
   /**
    * Checks if the two actions are equivalent. This method exists to support sharing actions between
    * configured targets for cases where there is no canonical target that could own the action. In
@@ -117,7 +135,7 @@ static boolean canBeSharedLogForPotentialFalsePositives(
               .findFirst();
       treeArtifactInput.ifPresent(
           treeArtifact ->
-              logger.atInfo().atMostEvery(5, TimeUnit.MINUTES).log(
+              logger.atInfo().atMostEvery(5, MINUTES).log(
                   "Shared action: %s has a tree artifact input: %s -- shared actions"
                       + " detection is overly permissive in this case and may allow"
                       + " sharing of different actions",
@@ -164,7 +182,7 @@ private static boolean artifactsEqualWithoutOwner(
   public static GeneratingActions assignOwnersAndFindAndThrowActionConflict(
       ActionKeyContext actionKeyContext,
       ImmutableList<ActionAnalysisMetadata> actions,
-      ActionLookupValue.ActionLookupKey actionLookupKey)
+      ActionLookupKey actionLookupKey)
       throws ActionConflictException {
     return Actions.assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict(
         actionKeyContext,
@@ -266,7 +284,10 @@ private static GeneratingActions assignOwnersAndMaybeFilterSharedActionsAndThrow
     // Loop over the actions, looking at all outputs for conflicts.
     int actionIndex = 0;
     for (ActionAnalysisMetadata action : actions) {
-      ActionLookupData generatingActionKey = ActionLookupData.create(actionLookupKey, actionIndex);
+      ActionLookupData generatingActionKey =
+          dependsOnBuildId(action)
+              ? ActionLookupData.createUnshareable(actionLookupKey, actionIndex)
+              : ActionLookupData.create(actionLookupKey, actionIndex);
       for (Artifact artifact : action.getOutputs()) {
         Preconditions.checkState(
             !artifact.isSourceArtifact(),
@@ -366,10 +387,10 @@ public int compare(PathFragment lhs, PathFragment rhs) {
    *     sorted using the comparator from {@link #comparatorForPrefixConflicts()}.
    * @param strictConflictChecks report path prefix conflicts, regardless of
    *     shouldReportPathPrefixConflict().
-   * @return A map between actions that generated the conflicting artifacts and their associated
-   *     {@link ArtifactPrefixConflictException}.
+   * @return An immutable map between actions that generated the conflicting artifacts and their
+   *     associated {@link ArtifactPrefixConflictException}.
    */
-  public static Map<ActionAnalysisMetadata, ArtifactPrefixConflictException>
+  public static ImmutableMap<ActionAnalysisMetadata, ArtifactPrefixConflictException>
       findArtifactPrefixConflicts(
           ActionGraph actionGraph,
           SortedMap<PathFragment, Artifact> artifactPathMap,
@@ -477,9 +498,9 @@ private GeneratingActions(
 
     /** Used only for the workspace status action. Does not handle duplicate artifacts. */
     public static GeneratingActions fromSingleAction(
-        ActionAnalysisMetadata action, ActionLookupValue.ActionLookupKey actionLookupKey) {
+        ActionAnalysisMetadata action, ActionLookupKey actionLookupKey) {
       Preconditions.checkState(actionLookupKey.getLabel() == null, actionLookupKey);
-      ActionLookupData generatingActionKey = ActionLookupData.create(actionLookupKey, 0);
+      ActionLookupData generatingActionKey = ActionLookupData.createUnshareable(actionLookupKey, 0);
       for (Artifact output : action.getOutputs()) {
         ((Artifact.DerivedArtifact) output).setGeneratingActionKey(generatingActionKey);
       }

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

@@ -62,6 +62,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_aware_action",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi",

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.actions.ActionInputMapSink;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionRewoundEvent;
+import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
@@ -146,7 +147,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       return null;
     }
     skyframeActionExecutor.noteActionEvaluationStarted(actionLookupData, action);
-    if (SkyframeActionExecutor.actionDependsOnBuildId(action)) {
+    if (Actions.dependsOnBuildId(action)) {
       PrecomputedValue.BUILD_ID.get(env);
     }
 
@@ -780,7 +781,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
           (action instanceof IncludeScannable)
               ? ((IncludeScannable) action).getDiscoveredModules()
               : null,
-          SkyframeActionExecutor.actionDependsOnBuildId(action));
+          Actions.dependsOnBuildId(action));
     }
 
     metadataHandler.prepareForActionExecution();

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

@@ -49,6 +49,7 @@
 import com.google.devtools.build.lib.actions.ActionResultReceivedEvent;
 import com.google.devtools.build.lib.actions.ActionScanningCompletedEvent;
 import com.google.devtools.build.lib.actions.ActionStartedEvent;
+import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl;
@@ -128,15 +129,6 @@
  * all output artifacts were created, error reporting, etc.
  */
 public final class SkyframeActionExecutor {
-  static boolean actionDependsOnBuildId(Action action) {
-    // Volatile build actions may need to execute even if none of their known inputs have changed.
-    // Depending on the build id ensures that these actions have a chance to execute.
-    // SkyframeAwareActions do not need to depend on the build id because their volatility is due to
-    // their dependence on Skyframe nodes that are not captured in the action cache. Any changes to
-    // those nodes will cause this action to be rerun, so a build id dependency is unnecessary.
-    return (action.isVolatile() && !(action instanceof SkyframeAwareAction))
-        || action instanceof NotifyOnActionCacheHit;
-  }
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
@@ -1183,7 +1175,7 @@ private ActionExecutionValue actuallyCompleteAction(
           (action instanceof IncludeScannable)
               ? ((IncludeScannable) action).getDiscoveredModules()
               : null,
-          actionDependsOnBuildId(action));
+          Actions.dependsOnBuildId(action));
     }
 
     /** A closure to continue an asynchronously running action. */

src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java

@@ -298,7 +298,7 @@ public void executeBuild(List<String> targets) throws Exception {
 
       env.beforeCommand(InvocationPolicy.getDefaultInstance());
 
-      lastRequest = createRequest("build", targets);
+      lastRequest = createRequest(env.getCommandName(), targets);
       lastResult = new BuildResult(lastRequest.getStartTime());
       boolean success = false;
 
@@ -341,14 +341,19 @@ private static FailureDetail createGenericDetailedFailure() {
   }
 
   BuildRequest createRequest(String commandName, List<String> targets) {
-    return BuildRequest.create(
-        commandName,
-        optionsParser,
-        null,
-        targets,
-        env.getReporter().getOutErr(),
-        env.getCommandId(),
-        runtime.getClock().currentTimeMillis());
+    BuildRequest request =
+        BuildRequest.create(
+            commandName,
+            optionsParser,
+            null,
+            targets,
+            env.getReporter().getOutErr(),
+            env.getCommandId(),
+            runtime.getClock().currentTimeMillis());
+    if ("test".equals(commandName)) {
+      request.setRunTests();
+    }
+    return request;
   }
 
   public BuildRequest getLastRequest() {