remote: add support for directory inputs in runfiles

Add support for tree artifacts (ctx.action.declare_directory(...)) in
runfiles. Before this change we would throw away the information
about the files inside a tree artifact before executing an action.
That's fine for local execution where the sandbox just copies/symlinks
a directory and doesn't care much what's inside. However, in remote
execution we actually need to upload each individual file and so
we need to be aware of all individual files not just directories.

This change makes it so that this information is made available to a
SpawnRunner via the SpawnInputExpander.

RELNOTES: None
PiperOrigin-RevId: 207091668

1 files changed, 90 insertions, 50 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 6d8a90d524..38fb99a283 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -74,14 +74,15 @@ import javax.annotation.Nullable;
 /**
  * A {@link SkyFunction} that creates {@link ActionExecutionValue}s. There are four points where
  * this function can abort due to missing values in the graph:
+ *
  * <ol>
  *   <li>For actions that discover inputs, if missing metadata needed to resolve an artifact from a
- *   string input in the action cache.</li>
- *   <li>If missing metadata for artifacts in inputs (including the artifacts above).</li>
+ *       string input in the action cache.
+ *   <li>If missing metadata for artifacts in inputs (including the artifacts above).
  *   <li>For actions that discover inputs, if missing metadata for inputs discovered prior to
- *   execution.</li>
+ *       execution.
  *   <li>For actions that discover inputs, but do so during execution, if missing metadata for
- *   inputs discovered during execution.</li>
+ *       inputs discovered during execution.
  * </ol>
  */
 public class ActionExecutionFunction implements SkyFunction, CompletionReceiver {
@@ -90,7 +91,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
   private final AtomicReference<TimestampGranularityMonitor> tsgm;
   private ConcurrentMap<Action, ContinuationState> stateMap;
 
-  public ActionExecutionFunction(SkyframeActionExecutor skyframeActionExecutor,
+  public ActionExecutionFunction(
+      SkyframeActionExecutor skyframeActionExecutor,
       BlazeDirectories directories,
       AtomicReference<TimestampGranularityMonitor> tsgm) {
     this.skyframeActionExecutor = skyframeActionExecutor;
@@ -100,8 +102,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
   }
 
   @Override
-  public SkyValue compute(SkyKey skyKey, Environment env) throws ActionExecutionFunctionException,
-      InterruptedException {
+  public SkyValue compute(SkyKey skyKey, Environment env)
+      throws ActionExecutionFunctionException, InterruptedException {
     ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument();
     ActionLookupValue actionLookupValue =
         (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey());
@@ -159,10 +161,14 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     try {
       // Declare deps on known inputs to action. We do this unconditionally to maintain our
       // invariant of asking for the same deps each build.
-      Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps
-          = env.getValuesOrThrow(toKeys(state.allInputs.getAllInputs(),
-              action.discoversInputs() ? action.getMandatoryInputs() : null),
-          MissingInputFileException.class, ActionExecutionException.class);
+      Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>>
+          inputDeps =
+              env.getValuesOrThrow(
+                  toKeys(
+                      state.allInputs.getAllInputs(),
+                      action.discoversInputs() ? action.getMandatoryInputs() : null),
+                  MissingInputFileException.class,
+                  ActionExecutionException.class);
 
       if (!sharedActionAlreadyRan && !state.hasArtifactData()) {
         // Do we actually need to find our metadata?
@@ -274,10 +280,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
   private static class AllInputs {
     final Iterable<Artifact> defaultInputs;
-    @Nullable
-    final Iterable<Artifact> actionCacheInputs;
-    @Nullable
-    final List<SkyKey> keysRequested;
+    @Nullable final Iterable<Artifact> actionCacheInputs;
+    @Nullable final List<SkyKey> keysRequested;
 
     AllInputs(Iterable<Artifact> defaultInputs) {
       this.defaultInputs = Preconditions.checkNotNull(defaultInputs);
@@ -285,7 +289,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       this.keysRequested = null;
     }
 
-    AllInputs(Iterable<Artifact> defaultInputs, Iterable<Artifact> actionCacheInputs,
+    AllInputs(
+        Iterable<Artifact> defaultInputs,
+        Iterable<Artifact> actionCacheInputs,
         List<SkyKey> keysRequested) {
       this.defaultInputs = Preconditions.checkNotNull(defaultInputs);
       this.actionCacheInputs = Preconditions.checkNotNull(actionCacheInputs);
@@ -314,13 +320,17 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     @Override
     public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
         throws InterruptedException {
-      Preconditions.checkState(keysRequested.isEmpty(),
-          "resolver should only be called once: %s %s", keysRequested, execPaths);
+      Preconditions.checkState(
+          keysRequested.isEmpty(),
+          "resolver should only be called once: %s %s",
+          keysRequested,
+          execPaths);
       // Create SkyKeys list based on execPaths.
       Map<PathFragment, SkyKey> depKeys = new HashMap<>();
       for (PathFragment path : execPaths) {
-        PathFragment parent = Preconditions.checkNotNull(
-            path.getParentDirectory(), "Must pass in files, not root directory");
+        PathFragment parent =
+            Preconditions.checkNotNull(
+                path.getParentDirectory(), "Must pass in files, not root directory");
         Preconditions.checkArgument(!parent.isAbsolute(), path);
         try {
           SkyKey depKey =
@@ -616,8 +626,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     }
   }
 
-  private static Iterable<SkyKey> toKeys(Iterable<Artifact> inputs,
-      Iterable<Artifact> mandatoryInputs) {
+  private static Iterable<SkyKey> toKeys(
+      Iterable<Artifact> inputs, Iterable<Artifact> mandatoryInputs) {
     if (mandatoryInputs == null) {
       // This is a non inputs-discovering action, so no need to distinguish mandatory from regular
       // inputs.
@@ -661,39 +671,44 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         new HashMap<>(populateInputData ? 128 : 0);
 
     ActionExecutionException firstActionExecutionException = null;
-    for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException,
-        ActionExecutionException>> depsEntry : inputDeps.entrySet()) {
+    for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>>
+        depsEntry : inputDeps.entrySet()) {
       Artifact input = ArtifactSkyKey.artifact(depsEntry.getKey());
       try {
         SkyValue value = depsEntry.getValue().get();
         if (populateInputData) {
           if (value instanceof AggregatingArtifactValue) {
             AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
-            for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getInputs()) {
+            for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getFileArtifacts()) {
               inputArtifactData.put(entry.first, entry.second);
             }
+            for (Pair<Artifact, TreeArtifactValue> entry : aggregatingValue.getTreeArtifacts()) {
+              expandTreeArtifactAndPopulateArtifactData(
+                  entry.getFirst(),
+                  Preconditions.checkNotNull(entry.getSecond()),
+                  expandedArtifacts,
+                  inputArtifactData);
+            }
             // We have to cache the "digest" of the aggregating value itself,
             // because the action cache checker may want it.
             inputArtifactData.put(input, aggregatingValue.getSelfData());
-            // Runfiles artifacts are not expanded into the action's inputs but their metadata is
-            // available from the action file cache.
+            // While not obvious at all this code exists to ensure that we don't expand the
+            // .runfiles/MANIFEST file into the inputs. The reason for that being that the MANIFEST
+            // file contains absolute paths that don't work with remote execution.
+            // Instead, the way the SpawnInputExpander expands runfiles is via the Runfiles class
+            // which contains all artifacts in the runfiles tree minus the MANIFEST file.
+            // TODO(buchgr): Clean this up and get rid of the RunfilesArtifactValue type.
             if (!(value instanceof RunfilesArtifactValue)) {
               ImmutableList.Builder<Artifact> expansionBuilder = ImmutableList.builder();
-              for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getInputs()) {
-                expansionBuilder.add(pair.first);
+              for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getFileArtifacts()) {
+                expansionBuilder.add(Preconditions.checkNotNull(pair.getFirst()));
               }
               expandedArtifacts.put(input, expansionBuilder.build());
             }
           } else if (value instanceof TreeArtifactValue) {
-            TreeArtifactValue treeValue = (TreeArtifactValue) value;
-            expandedArtifacts.put(input, ImmutableSet.<Artifact>copyOf(treeValue.getChildren()));
-            for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
-                treeValue.getChildValues().entrySet()) {
-              inputArtifactData.put(child.getKey(), child.getValue());
-            }
+            expandTreeArtifactAndPopulateArtifactData(
+                input, (TreeArtifactValue) value, expandedArtifacts, inputArtifactData);
 
-            // Again, we cache the "digest" of the value for cache checking.
-            inputArtifactData.put(input, treeValue.getSelfData());
           } else {
             Preconditions.checkState(value instanceof FileArtifactValue, depsEntry);
             inputArtifactData.put(input, (FileArtifactValue) value);
@@ -740,20 +755,37 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
                         "%s: missing input file '%s'",
                         action.getOwner().getLabel(), missingInput.getLabel())));
       }
-      throw new ActionExecutionException(missingCount + " input file(s) do not exist", action,
-          rootCauses.build(), /*catastrophe=*/false);
+      throw new ActionExecutionException(
+          missingCount + " input file(s) do not exist",
+          action,
+          rootCauses.build(),
+          /*catastrophe=*/ false);
     }
     return Pair.of(inputArtifactData, expandedArtifacts);
   }
 
+  private static void expandTreeArtifactAndPopulateArtifactData(
+      Artifact treeArtifact,
+      TreeArtifactValue value,
+      Map<Artifact, Collection<Artifact>> expandedArtifacts,
+      ActionInputMap inputArtifactData) {
+    ImmutableSet.Builder<Artifact> children = ImmutableSet.builder();
+    for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
+        value.getChildValues().entrySet()) {
+      children.add(child.getKey());
+      inputArtifactData.put(child.getKey(), child.getValue());
+    }
+    expandedArtifacts.put(treeArtifact, children.build());
+    // Again, we cache the "digest" of the value for cache checking.
+    inputArtifactData.put(treeArtifact, value.getSelfData());
+  }
+
   private static Iterable<Artifact> filterKnownInputs(
       Iterable<Artifact> newInputs, ActionInputMap inputArtifactData) {
     return Iterables.filter(newInputs, input -> inputArtifactData.getMetadata(input) == null);
   }
 
-  /**
-   * All info/warning messages associated with actions should be always displayed.
-   */
+  /** All info/warning messages associated with actions should be always displayed. */
   @Override
   public String extractTag(SkyKey skyKey) {
     return null;
@@ -783,15 +815,16 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
    * State to save work across restarts of ActionExecutionFunction due to missing values in the
    * graph for actions that discover inputs. There are three places where we save work, all for
    * actions that discover inputs:
+   *
    * <ol>
    *   <li>If not all known input metadata (coming from Action#getInputs) is available yet, then the
-   *   calculated set of inputs (including the inputs resolved from the action cache) is saved.</li>
+   *       calculated set of inputs (including the inputs resolved from the action cache) is saved.
    *   <li>If not all discovered inputs' metadata is available yet, then the known input metadata
-   *   together with the set of discovered inputs is saved, as well as the Token used to identify
-   *   this action to the action cache.</li>
+   *       together with the set of discovered inputs is saved, as well as the Token used to
+   *       identify this action to the action cache.
    *   <li>If, after execution, new inputs are discovered whose metadata is not yet available, then
-   *   the same data as in the previous case is saved, along with the actual result of execution.
-   *   </li>
+   *       the same data as in the previous case is saved, along with the actual result of
+   *       execution.
    * </ol>
    */
   private static class ContinuationState {
@@ -838,14 +871,21 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
     @Override
     public String toString() {
-      return token + ", " + value + ", " + allInputs + ", " + inputArtifactData + ", "
+      return token
+          + ", "
+          + value
+          + ", "
+          + allInputs
+          + ", "
+          + inputArtifactData
+          + ", "
           + discoveredInputs;
     }
   }
 
   /**
-   * Used to declare all the exception types that can be wrapped in the exception thrown by
-   * {@link ActionExecutionFunction#compute}.
+   * Used to declare all the exception types that can be wrapped in the exception thrown by {@link
+   * ActionExecutionFunction#compute}.
    */
   private static final class ActionExecutionFunctionException extends SkyFunctionException {