Remove special handling of middleman actions.

Now that the only type of middleman is the runfiles one, we don't need to jump through a lot of hoops we had to for other sorts of middlemen that did not represent actual inputs to actions.

MiddlemanType is kept for the time being for an easier rollback of this change, if need be.

RELNOTES: Actions that create runfiles trees are now considered regular actions. This means that they are now reported in statistics, critical paths and the like.
PiperOrigin-RevId: 690642037
Change-Id: I472df8a8ad8235e5c8f616c8e354a5fd4739630e

Author: lberki

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

@@ -42,7 +42,6 @@
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
 import com.google.devtools.build.lib.vfs.OutputPermissions;
-import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -231,8 +230,8 @@ private static boolean validateArtifacts(
       }
     }
     for (Artifact artifact : actionInputs.toList()) {
-      mdMap.put(
-          artifact.getExecPathString(), getInputMetadataMaybe(inputMetadataProvider, artifact));
+      FileArtifactValue inputMetadata = getInputMetadataMaybe(inputMetadataProvider, artifact);
+      mdMap.put(artifact.getExecPathString(), inputMetadata);
     }
     return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest());
   }
@@ -606,6 +605,7 @@ private boolean mustExecute(
       actionCache.accountMiss(MissReason.UNCONDITIONAL_EXECUTION);
       return true;
     }
+
     if (entry == null) {
       reportNewAction(handler, action);
       actionCache.accountMiss(MissReason.NOT_CACHED);
@@ -657,7 +657,7 @@ private static FileArtifactValue getInputMetadataOrConstant(
       InputMetadataProvider inputMetadataProvider, Artifact artifact) throws IOException {
     FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(artifact);
     return (metadata != null && artifact.isConstantMetadata())
-        ? ConstantMetadataValue.INSTANCE
+        ? FileArtifactValue.ConstantMetadataValue.INSTANCE
         : metadata;
   }
 
@@ -666,7 +666,7 @@ private static FileArtifactValue getOutputMetadataOrConstant(
       throws IOException, InterruptedException {
     FileArtifactValue metadata = outputMetadataStore.getOutputMetadata(artifact);
     return (metadata != null && artifact.isConstantMetadata())
-        ? ConstantMetadataValue.INSTANCE
+        ? FileArtifactValue.ConstantMetadataValue.INSTANCE
         : metadata;
   }
 
@@ -992,44 +992,4 @@ private Token(Action action) {
     }
   }
 
-  private static final class ConstantMetadataValue extends FileArtifactValue
-      implements FileArtifactValue.Singleton {
-    static final ConstantMetadataValue INSTANCE = new ConstantMetadataValue();
-    // This needs to not be of length 0, so it is distinguishable from a missing digest when written
-    // into a Fingerprint.
-    private static final byte[] DIGEST = new byte[1];
-
-    private ConstantMetadataValue() {}
-
-    @Override
-    public FileStateType getType() {
-      return FileStateType.REGULAR_FILE;
-    }
-
-    @Override
-    public byte[] getDigest() {
-      return DIGEST;
-    }
-
-    @Override
-    public FileContentsProxy getContentsProxy() {
-      throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public long getSize() {
-      return 0;
-    }
-
-    @Override
-    public long getModifiedTime() {
-      return -1;
-    }
-
-    @Override
-    public boolean wasModifiedSinceDigest(Path path) {
-      throw new UnsupportedOperationException(
-          "ConstantMetadataValue doesn't support wasModifiedSinceDigest " + path.toString());
-    }
-  }
 }

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

@@ -58,7 +58,6 @@ java_library(
         "ActionKeyContext.java",
         "ActionLogBufferPathGenerator.java",
         "ActionLookupValue.java",
-        "ActionMiddlemanEvent.java",
         "ActionOwner.java",
         "ActionProgressEvent.java",
         "ActionRegistry.java",
@@ -436,6 +435,7 @@ java_library(
         ":file_metadata",
         ":runfiles_tree",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
+        "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:hash_codes",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//third_party:guava",

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

@@ -972,4 +972,46 @@ public String toString() {
       return "singleton marker artifact value (" + hashCode() + ")";
     }
   }
+
+  /** {@link FileArtifactValue} subclass for artifacts with constant metadata. A singleton. */
+  public static final class ConstantMetadataValue extends FileArtifactValue
+      implements FileArtifactValue.Singleton {
+    static final ConstantMetadataValue INSTANCE = new ConstantMetadataValue();
+    // This needs to not be of length 0, so it is distinguishable from a missing digest when written
+    // into a Fingerprint.
+    private static final byte[] DIGEST = new byte[1];
+
+    private ConstantMetadataValue() {}
+
+    @Override
+    public FileStateType getType() {
+      return FileStateType.REGULAR_FILE;
+    }
+
+    @Override
+    public byte[] getDigest() {
+      return DIGEST;
+    }
+
+    @Override
+    public FileContentsProxy getContentsProxy() {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public long getSize() {
+      return 0;
+    }
+
+    @Override
+    public long getModifiedTime() {
+      return -1;
+    }
+
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) {
+      throw new UnsupportedOperationException(
+          "ConstantMetadataValue doesn't support wasModifiedSinceDigest " + path);
+    }
+  }
 }

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

@@ -22,6 +22,8 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.BulkDeleter;
+import com.google.devtools.build.lib.vfs.Path;
 import javax.annotation.Nullable;
 
 /**
@@ -53,7 +55,18 @@ public RunfilesTree getRunfilesTree() {
 
   @Override
   public ActionResult execute(ActionExecutionContext actionExecutionContext) {
-    throw new IllegalStateException("MiddlemanAction should never be executed");
+    return ActionResult.EMPTY;
+  }
+
+  @Override
+  public void prepare(
+      Path execRoot,
+      ArtifactPathResolver pathResolver,
+      @Nullable BulkDeleter bulkDeleter,
+      boolean cleanupArchivedArtifacts) {
+    // Runfiles trees are created as a side effect of building the output manifest, not the runfiles
+    // tree artifact. This method is overridden so that depending on the runfiles middleman does not
+    // delete the runfiles tree that's on the file system that someone decided it must be there.
   }
 
   @Override

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

@@ -13,7 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-/** The action type. */
+/** Deprecated, slated to be removed. */
 public enum MiddlemanType {
 
   /** A normal action. */
@@ -27,6 +27,9 @@ public enum MiddlemanType {
   RUNFILES_MIDDLEMAN;
 
   public boolean isMiddleman() {
-    return this != NORMAL;
+    // This value is always false, which means that in theory, the MiddlemanType enum is not useful
+    // anymore. It's kept here to facilitate an easy rollback for the change that made the enum
+    // unnecessary should trouble arise.
+    return false;
   }
 }

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

@@ -18,7 +18,9 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.FileArtifactValue.ConstantMetadataValue;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.HashCodes;
 import com.google.devtools.build.skyframe.SkyValue;
 
@@ -43,13 +45,11 @@ public interface RunfilesConsumer<T> {
   private final ImmutableList<TreeArtifactValue> treeValues;
 
   public RunfilesArtifactValue(
-      FileArtifactValue metadata,
       RunfilesTree runfilesTree,
       ImmutableList<Artifact> files,
       ImmutableList<FileArtifactValue> fileValues,
       ImmutableList<Artifact> trees,
       ImmutableList<TreeArtifactValue> treeValues) {
-    this.metadata = checkNotNull(metadata);
     this.runfilesTree = checkNotNull(runfilesTree);
     this.files = checkNotNull(files);
     this.fileValues = checkNotNull(fileValues);
@@ -59,10 +59,41 @@ public RunfilesArtifactValue(
         files.size() == fileValues.size() && trees.size() == treeValues.size(),
         "Size mismatch: %s",
         this);
+
+    // Compute the digest of this runfiles tree by combining its layout and the digests of every
+    // artifact it references.
+    this.metadata = FileArtifactValue.createProxy(computeDigest());
+  }
+
+  private byte[] computeDigest() {
+    Fingerprint result = new Fingerprint();
+
+    result.addInt(runfilesTree.getMapping().size());
+    for (var entry : runfilesTree.getMapping().entrySet()) {
+      result.addPath(entry.getKey());
+      result.addBoolean(entry.getValue() != null);
+      if (entry.getValue() != null) {
+        result.addPath(entry.getValue().getExecPath());
+      }
+    }
+
+    result.addInt(files.size());
+    for (int i = 0; i < files.size(); i++) {
+      FileArtifactValue value =
+          files.get(i).isConstantMetadata() ? ConstantMetadataValue.INSTANCE : fileValues.get(i);
+      value.addTo(result);
+    }
+
+    result.addInt(trees.size());
+    for (int i = 0; i < trees.size(); i++) {
+      result.addBytes(treeValues.get(i).getDigest());
+    }
+
+    return result.digestAndReset();
   }
 
   public RunfilesArtifactValue withOverriddenRunfilesTree(RunfilesTree overrideTree) {
-    return new RunfilesArtifactValue(metadata, overrideTree, files, fileValues, trees, treeValues);
+    return new RunfilesArtifactValue(overrideTree, files, fileValues, trees, treeValues);
   }
 
   /** Returns the data of the artifact for this value, as computed by the action cache checker. */

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

@@ -19,7 +19,6 @@
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
 import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.MiddlemanType;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.skyframe.ActionExecutionInactivityWatchdog;
 import com.google.devtools.build.lib.skyframe.AspectCompletionValue;
@@ -184,7 +183,7 @@ public void actionCompleted(ActionLookupData actionLookupData) {
   }
 
   private static boolean isActionReportWorthy(Action action) {
-    return action.getActionType() == MiddlemanType.NORMAL;
+    return !action.getActionType().isMiddleman();
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java

@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionCompletionEvent;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
-import com.google.devtools.build.lib.actions.ActionMiddlemanEvent;
 import com.google.devtools.build.lib.actions.ActionStartedEvent;
 import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.AggregatedSpawnMetrics;
@@ -234,23 +233,6 @@ public void actionStarted(ActionStartedEvent event) throws InterruptedException
     tryAddComponent(createComponent(action, event.getNanoTimeStart())).startRunning();
   }
 
-  /**
-   * Record a middleman action execution. Even if middleman are almost instant, we record them
-   * because they depend on other actions and we need them for constructing the critical path.
-   *
-   * <p>For some rules with incorrect configuration transitions we might get notified several times
-   * for the same middleman. This should only happen if the actions are shared.
-   */
-  @Subscribe
-  @AllowConcurrentEvents
-  public void middlemanAction(ActionMiddlemanEvent event) throws InterruptedException {
-    Action action = event.getAction();
-    CriticalPathComponent component =
-        tryAddComponent(createComponent(action, event.getNanoTimeStart()));
-    finalizeActionStat(
-        event.getNanoTimeStart(), event.getNanoTimeFinish(), action, component, "middleman action");
-  }
-
   /**
    * Try to add the component to the map of critical path components. If there is an existing
    * component for its primary output it uses that to update the rest of the outputs.