Put a reference to RunfilesTree into MiddlemanAction and simplify the…

… creation of the latter.

The plan is that it will be plumbed to RunfilesArtifactValue, thereby making RunfilesSupplier unnecessary.

I don't know yet how that will work; the most likely approach is an ugly downcast of Action to MiddlemanAction in ArtifactFunction, but I hope I can come up with something more palatable. So for now, the private field remains unused.

Since now there is only one kind of middleman, it was possible to delete a pleasant amount of code, including MiddlemanActionTest.

RELNOTES: None.
PiperOrigin-RevId: 597526236
Change-Id: I392fc662288cc5dbee3a058d246182916343f8cc

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

@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -33,25 +34,23 @@
 @Immutable
 public final class MiddlemanAction extends AbstractAction {
   public static final String MIDDLEMAN_MNEMONIC = "Middleman";
-  private final String description;
-  private final MiddlemanType middlemanType;
 
-  private MiddlemanAction(
+  /** The runfiles tree this middleman stands for. */
+  private final RunfilesTree runfilesTree;
+
+  public MiddlemanAction(
       ActionOwner owner,
+      RunfilesTree runfilesTree,
       NestedSet<Artifact> inputs,
-      ImmutableSet<Artifact> outputs,
-      String description,
-      MiddlemanType middlemanType) {
+      ImmutableSet<Artifact> outputs) {
     super(owner, inputs, outputs);
-    Preconditions.checkNotNull(middlemanType);
+
+    this.runfilesTree = runfilesTree;
     Preconditions.checkArgument(Iterables.getOnlyElement(outputs).isMiddlemanArtifact(), outputs);
-    Preconditions.checkNotNull(description);
-    this.description = description;
-    this.middlemanType = middlemanType;
   }
 
   @Override
-  public final ActionResult execute(ActionExecutionContext actionExecutionContext) {
+  public ActionResult execute(ActionExecutionContext actionExecutionContext) {
     throw new IllegalStateException("MiddlemanAction should never be executed");
   }
 
@@ -70,7 +69,7 @@ protected void computeKey(
    */
   @Override
   public MiddlemanType getActionType() {
-    return middlemanType;
+    return MiddlemanType.RUNFILES_MIDDLEMAN;
   }
 
   @Nullable
@@ -81,7 +80,7 @@ protected String getRawProgressMessage() {
 
   @Override
   public String prettyPrint() {
-    return description + " for " + Label.print(getOwner().getLabel());
+    return "runfiles for " + Label.print(getOwner().getLabel());
   }
 
   @Override
@@ -106,18 +105,4 @@ public ImmutableMap<String, String> getExecProperties() {
     // Middleman actions do not execute actual actions, and therefore have no execution properties.
     return ImmutableMap.of();
   }
-
-  /** Creates a new middleman action. */
-  public static Action create(
-      ActionRegistry env,
-      ActionOwner owner,
-      NestedSet<Artifact> inputs,
-      Artifact stampFile,
-      String purpose,
-      MiddlemanType middlemanType) {
-    MiddlemanAction action =
-        new MiddlemanAction(owner, inputs, ImmutableSet.of(stampFile), purpose, middlemanType);
-    env.registerAction(action);
-    return action;
-  }
 }

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

@@ -14,10 +14,12 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
 
@@ -48,59 +50,44 @@ public MiddlemanFactory(
    *     Further, if the owning Artifact is non-null, the owning Artifacts' root-relative path must
    *     be unique and the artifact must be part of the runfiles tree for which this middleman is
    *     created. Usually this artifact will be an executable program.
-   * @param inputs the set of artifacts for which the created artifact is to be the middleman.
+   * @param runfilesTree the runfiles tree for which the middleman being created for
+   * @param runfilesManifest the runfiles manifest for the runfiles tree
+   * @param repoMappingManifest the repo mapping manifest for the runfiles tree
    * @param middlemanDir the directory in which to place the middleman.
    */
   public Artifact createRunfilesMiddleman(
       ActionOwner owner,
       @Nullable Artifact owningArtifact,
-      NestedSet<Artifact> inputs,
-      ArtifactRoot middlemanDir,
-      String tag) {
+      RunfilesTree runfilesTree,
+      @Nullable Artifact runfilesManifest,
+      @Nullable Artifact repoMappingManifest,
+      ArtifactRoot middlemanDir) {
     Preconditions.checkArgument(middlemanDir.isMiddlemanRoot());
-    if (inputs.isSingleton()) { // Optimization: No middleman for just one input.
-      return inputs.getSingleton();
+
+    NestedSetBuilder<Artifact> depsBuilder = NestedSetBuilder.stableOrder();
+    depsBuilder.addTransitive(runfilesTree.getArtifacts());
+    if (runfilesManifest != null) {
+      depsBuilder.add(runfilesManifest);
+    }
+    if (repoMappingManifest != null) {
+      depsBuilder.add(repoMappingManifest);
+    }
+
+    NestedSet<Artifact> deps = depsBuilder.build();
+    if (deps.isSingleton()) { // Optimization: No middleman for just one input.
+      return deps.getSingleton();
     }
     String middlemanPath = owningArtifact == null
        ? Label.print(owner.getLabel())
        : owningArtifact.getRootRelativePath().getPathString();
-    return createMiddleman(owner, middlemanPath, tag, inputs, middlemanDir,
-        MiddlemanType.RUNFILES_MIDDLEMAN).getFirst();
-  }
-
-  /**
-   * Creates middlemen.
-   *
-   * <p>Note: there's no need to synchronize this method; the only use of a field is via a call to
-   * another synchronized method (getArtifact()).
-   *
-   * @return null iff {@code inputs} is null or empty; the middleman file and the middleman action
-   *     otherwise
-   */
-  @Nullable
-  private Pair<Artifact, Action> createMiddleman(
-      ActionOwner owner,
-      String middlemanName,
-      String purpose,
-      NestedSet<Artifact> inputs,
-      ArtifactRoot middlemanDir,
-      MiddlemanType middlemanType) {
-    if (inputs == null || inputs.isEmpty()) {
-      return null;
-    }
-
-    Artifact stampFile = getStampFileArtifact(middlemanName, purpose, middlemanDir);
-    Action action =
-        MiddlemanAction.create(actionRegistry, owner, inputs, stampFile, purpose, middlemanType);
-    return Pair.of(stampFile, action);
-  }
-
-  private Artifact.DerivedArtifact getStampFileArtifact(
-      String middlemanName, String purpose, ArtifactRoot middlemanDir) {
-    String escapedFilename = Actions.escapedPath(middlemanName);
-    PathFragment stampName = PathFragment.create("_middlemen/" + escapedFilename + "-" + purpose);
+    String escapedFilename = Actions.escapedPath(middlemanPath);
+    PathFragment stampName = PathFragment.create("_middlemen/" + escapedFilename + "-runfiles");
     Artifact.DerivedArtifact stampFile =
         artifactFactory.getDerivedArtifact(stampName, middlemanDir, actionRegistry.getOwner());
+    MiddlemanAction middlemanAction =
+        new MiddlemanAction(owner, runfilesTree, deps, ImmutableSet.of(stampFile));
+    actionRegistry.registerAction(middlemanAction);
     return stampFile;
   }
+
 }

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -31,7 +31,6 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.Type;
@@ -210,7 +209,7 @@ private static RunfilesSupport create(
 
     Artifact runfilesMiddleman =
         createRunfilesMiddleman(
-            ruleContext, owningExecutable, runfiles, runfilesManifest, repoMappingManifest);
+            ruleContext, owningExecutable, runfilesTree, runfilesManifest, repoMappingManifest);
 
     return new RunfilesSupport(
         runfilesTree,
@@ -346,26 +345,19 @@ public Artifact getRunfilesMiddleman() {
   private static Artifact createRunfilesMiddleman(
       ActionConstructionContext context,
       Artifact owningExecutable,
-      Runfiles runfiles,
+      RunfilesTree runfilesTree,
       @Nullable Artifact runfilesManifest,
       Artifact repoMappingManifest) {
-    NestedSetBuilder<Artifact> deps = NestedSetBuilder.stableOrder();
-    deps.addTransitive(runfiles.getAllArtifacts());
-    if (runfilesManifest != null) {
-      deps.add(runfilesManifest);
-    }
-    if (repoMappingManifest != null) {
-      deps.add(repoMappingManifest);
-    }
     return context
         .getAnalysisEnvironment()
         .getMiddlemanFactory()
         .createRunfilesMiddleman(
             context.getActionOwner(),
             owningExecutable,
-            deps.build(),
-            context.getMiddlemanDirectory(),
-            "runfiles");
+            runfilesTree,
+            runfilesManifest,
+            repoMappingManifest,
+            context.getMiddlemanDirectory());
   }
 
   /**