Unify sandbox/remote handling of empty TreeArtifact inputs

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

@@ -103,12 +103,17 @@ public PathFragment getExecPath() {
   }
 
   /**
-   * Expands middleman artifacts in a sequence of {@link ActionInput}s.
+   * Expands middleman and tree artifacts in a sequence of {@link ActionInput}s.
    *
-   * <p>Non-middleman artifacts are returned untouched.
+   * The returned list never contains middleman artifacts. It contains tree artifacts only if
+   * {@code keepEmptyTreeArtifacts} is true. In this case, all returned tree artifacts have empty
+   * expansion under the provided {@link ArtifactExpander}.
+   *
+   * <p>Non-middleman, non-tree artifacts are returned untouched.
    */
   public static List<ActionInput> expandArtifacts(
-      NestedSet<? extends ActionInput> inputs, ArtifactExpander artifactExpander) {
+      NestedSet<? extends ActionInput> inputs, ArtifactExpander artifactExpander,
+      boolean keepEmptyTreeArtifacts) {
     List<ActionInput> result = new ArrayList<>();
     List<Artifact> containedArtifacts = new ArrayList<>();
     for (ActionInput input : inputs.toList()) {
@@ -118,7 +123,8 @@ public static List<ActionInput> expandArtifacts(
       }
       containedArtifacts.add((Artifact) input);
     }
-    Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander);
+    Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander,
+        keepEmptyTreeArtifacts);
     return result;
   }
 

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

@@ -1520,26 +1520,39 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
     return Joiner.on(delimiter).join(toRootRelativePaths(artifacts));
   }
 
-  /** Adds a collection of artifacts to a given collection, with middleman actions expanded once. */
+  /**
+   * Adds a collection of artifacts to a given collection, with middleman actions and tree artifacts
+   * expanded once.
+   *
+   * The constructed list never contains middleman artifacts. It contains tree artifacts only if
+   * {@code keepEmptyTreeArtifacts} is true. In this case, all added tree artifacts have empty
+   * expansion under the provided {@link ArtifactExpander}.
+   */
   static void addExpandedArtifacts(
       Iterable<Artifact> artifacts,
       Collection<? super Artifact> output,
-      ArtifactExpander artifactExpander) {
-    addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander);
+      ArtifactExpander artifactExpander,
+      boolean keepEmptyTreeArtifacts) {
+    addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander,
+        keepEmptyTreeArtifacts);
   }
 
   /**
-   * Converts a collection of artifacts into the outputs computed by
-   * outputFormatter and adds them to a given collection. Middleman artifacts
-   * are expanded once.
+   * Converts a collection of artifacts into the outputs computed by outputFormatter and adds them
+   * to a given collection. Middleman artifacts and tree artifacts are expanded once.
+   *
+   * The constructed list never contains middleman artifacts. It contains tree artifacts only if
+   * {@code keepEmptyTreeArtifacts} is true. In this case, all added tree artifacts have empty
+   * expansion under the provided {@link ArtifactExpander}.
    */
   private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifacts,
-                                               Collection<? super E> output,
-                                               Function<? super Artifact, E> outputFormatter,
-                                               ArtifactExpander artifactExpander) {
+      Collection<? super E> output,
+      Function<? super Artifact, E> outputFormatter,
+      ArtifactExpander artifactExpander,
+      boolean keepEmptyTreeArtifacts) {
     for (Artifact artifact : artifacts) {
       if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
-        expandArtifact(artifact, output, outputFormatter, artifactExpander);
+        expandArtifact(artifact, output, outputFormatter, artifactExpander, keepEmptyTreeArtifacts);
       } else {
         output.add(outputFormatter.apply(artifact));
       }
@@ -1549,13 +1562,17 @@ private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifa
   private static <E> void expandArtifact(Artifact middleman,
       Collection<? super E> output,
       Function<? super Artifact, E> outputFormatter,
-      ArtifactExpander artifactExpander) {
+      ArtifactExpander artifactExpander,
+      boolean keepEmptyTreeArtifacts) {
     Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact());
     List<Artifact> artifacts = new ArrayList<>();
     artifactExpander.expand(middleman, artifacts);
     for (Artifact artifact : artifacts) {
       output.add(outputFormatter.apply(artifact));
     }
+    if (keepEmptyTreeArtifacts && middleman.isTreeArtifact() && artifacts.isEmpty()) {
+      output.add(outputFormatter.apply(middleman));
+    }
   }
 
   /**

src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java

@@ -130,7 +130,8 @@ void addRunfilesToInputs(
           if (localArtifact.isTreeArtifact()) {
             List<ActionInput> expandedInputs =
                 ActionInputHelper.expandArtifacts(
-                    NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander);
+                    NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander,
+                    /* keepEmptyTreeArtifacts= */ false);
             for (ActionInput input : expandedInputs) {
               addMapping(
                   inputMap,
@@ -222,16 +223,22 @@ private static void addInputs(
       NestedSet<? extends ActionInput> inputFiles,
       ArtifactExpander artifactExpander,
       PathFragment baseDirectory) {
-    List<ActionInput> inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander);
+    // Actions that accept TreeArtifacts as inputs generally expect the directory corresponding
+    // to the artifact to be created, even if it is empty. We explicitly keep empty TreeArtifacts
+    // here to signal consumers that they should create the directory.
+    List<ActionInput> inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander,
+        /* keepEmptyTreeArtifacts= */ true);
     for (ActionInput input : inputs) {
       addMapping(inputMap, input.getExecPath(), input, baseDirectory);
     }
   }
 
   /**
    * Convert the inputs and runfiles of the given spawn to a map from exec-root relative paths to
-   * {@link ActionInput}s. The returned map does not contain tree artifacts as they are expanded to
-   * file artifacts.
+   * {@link ActionInput}s. The returned map does not contain non-empty tree artifacts as they are
+   * expanded to file artifacts. Tree artifacts that would expand to the empty set under the
+   * provided {@link ArtifactExpander} are left untouched so that their corresponding empty
+   * directories can be created.
    *
    * <p>The returned map never contains {@code null} values.
    *

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -586,7 +586,6 @@ private static Spawn createCoveragePostProcessingSpawn(
             .addTransitive(action.getInputs())
             .addAll(expandedCoverageDir)
             .add(action.getCollectCoverageScript())
-            .add(action.getCoverageDirectoryTreeArtifact())
             .add(action.getCoverageManifest())
             .addTransitive(action.getLcovMergerFilesToRun().build())
             .build(),

src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD

@@ -20,6 +20,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util:string",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",

src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java

@@ -17,12 +17,14 @@
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
 import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -33,6 +35,7 @@
 import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
+import javax.annotation.Nullable;
 
 /** Builder for directory trees. */
 class DirectoryTreeBuilder {
@@ -94,6 +97,7 @@ private static int buildFromPaths(
       Map<PathFragment, DirectoryNode> tree)
       throws IOException {
     return build(
+        null,
         inputs,
         tree,
         (input, path, currDir) -> {
@@ -122,6 +126,7 @@ private static int buildFromActionInputs(
       Map<PathFragment, DirectoryNode> tree)
       throws IOException {
     return build(
+        metadataProvider,
         inputs,
         tree,
         (input, path, currDir) -> {
@@ -177,6 +182,7 @@ private static int buildFromActionInputs(
   }
 
   private static <T> int build(
+      @Nullable MetadataProvider metadataProvider,
       SortedMap<PathFragment, T> inputs,
       Map<PathFragment, DirectoryNode> tree,
       FileNodeVisitor<T> fileNodeVisitor)
@@ -192,6 +198,30 @@ private static <T> int build(
       // Path relative to the exec root
       PathFragment path = e.getKey();
       T input = e.getValue();
+
+      if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) {
+        DerivedArtifact artifact = (DerivedArtifact) input;
+        // MetadataProvider is provided by all callers for which T is a superclass of
+        // DerivedArtifact.
+        Preconditions.checkNotNull(metadataProvider);
+        FileArtifactValue metadata =
+            Preconditions.checkNotNull(
+                metadataProvider.getMetadata(artifact),
+                "missing metadata for '%s'",
+                artifact.getExecPathString());
+        Preconditions.checkState(metadata.equals(TreeArtifactValue.empty().getMetadata()),
+            "Encountered non-empty TreeArtifact '%s' with metadata '%s', which should have"
+                + " been expanded by SpawnInputExpander. This is a bug.",
+            path, metadata);
+        // Create an empty directory and its parent directories but don't visit the TreeArtifact
+        // input itself: A TreeArtifact's metadata has type REGULAR_FILE, not DIRECTORY, and would
+        // thus lead to an empty file being created in the buildFromActionInputs visitor.
+        DirectoryNode emptyDir = new DirectoryNode(path.getBaseName());
+        tree.put(path, emptyDir);
+        createParentDirectoriesIfNotExist(path, emptyDir, tree);
+        continue;
+      }
+
       if (dirname == null || !path.getParentDirectory().equals(dirname)) {
         dirname = path.getParentDirectory();
         dir = tree.get(dirname);

src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java

@@ -254,7 +254,8 @@ private static MerkleTree build(DirectoryTree tree, DigestUtil digestUtil) {
           for (DirectoryTree.DirectoryNode dir : dirs) {
             PathFragment subDirname = dirname.getRelative(dir.getPathSegment());
             MerkleTree subMerkleTree =
-                Preconditions.checkNotNull(m.remove(subDirname), "subMerkleTree was null");
+                Preconditions.checkNotNull(m.remove(subDirname), "subMerkleTree at '%s' was null",
+                    subDirname);
             subDirs.put(dir.getPathSegment(), subMerkleTree);
           }
           MerkleTree mt = buildMerkleTree(new TreeSet<>(files), subDirs, digestUtil);

src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

@@ -233,8 +233,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            spawn,
-            context.getArtifactExpander(),
             execRoot);
     SandboxOutputs outputs = helpers.getOutputs(spawn);
 

src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java

@@ -223,8 +223,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            spawn,
-            context.getArtifactExpander(),
             execRoot);
     SandboxOutputs outputs = helpers.getOutputs(spawn);
 

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

@@ -184,8 +184,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            spawn,
-            context.getArtifactExpander(),
             execRoot);
     SandboxOutputs outputs = helpers.getOutputs(spawn);
 

src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

@@ -112,8 +112,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            spawn,
-            context.getArtifactExpander(),
             execRoot);
     SandboxOutputs outputs = helpers.getOutputs(spawn);
 

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -27,7 +27,6 @@
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
@@ -42,10 +41,8 @@
 import com.google.devtools.common.options.OptionsParsingResult;
 import java.io.IOException;
 import java.io.OutputStream;
-import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
@@ -487,27 +484,8 @@ private static void writeVirtualInputTo(VirtualActionInput input, Path target)
    */
   public SandboxInputs processInputFiles(
       Map<PathFragment, ActionInput> inputMap,
-      Spawn spawn,
-      ArtifactExpander artifactExpander,
       Path execRoot)
       throws IOException {
-    // SpawnInputExpander#getInputMapping uses ArtifactExpander#expandArtifacts to expand
-    // middlemen and tree artifacts, which expands empty tree artifacts to no entry. However,
-    // actions that accept TreeArtifacts as inputs generally expect that the empty directory is
-    // created. So we add those explicitly here.
-    // TODO(ulfjack): Move this code to SpawnInputExpander.
-    for (ActionInput input : spawn.getInputFiles().toList()) {
-      if (input instanceof Artifact && ((Artifact) input).isTreeArtifact()) {
-        List<Artifact> containedArtifacts = new ArrayList<>();
-        artifactExpander.expand((Artifact) input, containedArtifacts);
-        // Attempting to mount a non-empty directory results in ERR_DIRECTORY_NOT_EMPTY, so we
-        // only mount empty TreeArtifacts as directories.
-        if (containedArtifacts.isEmpty()) {
-          inputMap.put(input.getExecPath(), input);
-        }
-      }
-    }
-
     Map<PathFragment, Path> inputFiles = new TreeMap<>();
     Set<VirtualActionInput> virtualInputs = new HashSet<>();
     Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();

src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java

@@ -72,8 +72,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs readablePaths =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            spawn,
-            context.getArtifactExpander(),
             execRoot);
 
     readablePaths.materializeVirtualInputs(execRoot);

src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java

@@ -64,7 +64,8 @@ public static SortedMap<PathFragment, byte[]> getWorkerFilesWithDigests(
     TreeMap<PathFragment, byte[]> workerFilesMap = new TreeMap<>();
 
     List<ActionInput> tools =
-        ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander);
+        ActionInputHelper.expandArtifacts(spawn.getToolFiles(),
+            artifactExpander, /* keepEmptyTreeArtifacts= */ false);
     for (ActionInput tool : tools) {
       @Nullable FileArtifactValue metadata = actionInputFileCache.getMetadata(tool);
       if (metadata == null) {

src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java

@@ -202,8 +202,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
         inputFiles =
             helpers.processInputFiles(
                 context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-                spawn,
-                context.getArtifactExpander(),
                 execRoot);
       }
       SandboxOutputs outputs = helpers.getOutputs(spawn);
@@ -259,7 +257,8 @@ private WorkRequest createWorkRequest(
     }
 
     List<ActionInput> inputs =
-        ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander());
+        ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander(),
+            /* keepEmptyTreeArtifacts= */ false);
 
     for (ActionInput input : inputs) {
       byte[] digestBytes = inputFileCache.getMetadata(input).getDigest();