Unify sandbox/remote handling of empty TreeArtifact inputs (bazelbuil…

…d#15449) Actions that take a TreeArtifact as input should see a corresponding directory even if the TreeArtifact is empty. Previously, SandboxHelpers contained special handling for the case of empty TreeArtifact action inputs to ensure that they are added to the sandbox as empty directories. As pointed out in a comment, this logic should live in SpawnInputExpander, where it would also apply to remote execution. This commit adds a integration tests for this previously untested case, extracts the logic into SpawnInputExpander and adapts DirectoryTreeBuilder to handle empty TreeArtifacts when creating the Merkle trees. Note: The Merkle tree builder now reports an error when it encounters a non-empty TreeArtifact. Such an artifact should have been expanded by SpawnInputExpander and if it wasn't, e.g. because it wasn't properly registered with Skyframe, it can't be expanded at this point anyway. The tests uncovered that the spawn for split coverage postprocessing declared the coverage dir artifact as such an input. In this case, it can simply be removed as the coverage script creates the coverage dir if it doesn't exist. Closes bazelbuild#15276. PiperOrigin-RevId: 446452587 Co-authored-by: Fabian Meumertzheim <[email protected]>
aiuto · May 10, 2022 · 26f8783 · 26f8783
1 parent 027e9b2
commit 26f8783
Show file tree

Hide file tree

Showing 18 changed files with 212 additions and 135 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
@@ -103,12 +103,19 @@ 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.
+   * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+   * is true, a tree artifact will be included in the constructed list when it expands into zero
+   * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+   * included.
+   *
+   * <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 +125,8 @@ public static List<ActionInput> expandArtifacts(
       }
       containedArtifacts.add((Artifact) input);
     }
-    Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander);
+    Artifact.addExpandedArtifacts(
+        containedArtifacts, result, artifactExpander, keepEmptyTreeArtifacts);
     return result;
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -1566,42 +1566,63 @@ 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.
+   *
+   * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+   * is true, a tree artifact will be included in the constructed list when it expands into zero
+   * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+   * included.
+   */
   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.
+   *
+   * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+   * is true, a tree artifact will be included in the constructed list when it expands into zero
+   * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+   * included.
    */
-  private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifacts,
-                                               Collection<? super E> output,
-                                               Function<? super Artifact, E> outputFormatter,
-                                               ArtifactExpander artifactExpander) {
+  private static <E> void addExpandedArtifacts(
+      Iterable<? extends Artifact> artifacts,
+      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));
       }
     }
   }
 
-  private static <E> void expandArtifact(Artifact middleman,
+  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));
+    }
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
@@ -130,7 +130,9 @@ 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 +224,23 @@ 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.
    *

diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -591,7 +591,6 @@ private static Spawn createCoveragePostProcessingSpawn(
             .addTransitive(action.getInputs())
             .addAll(expandedCoverageDir)
             .add(action.getCollectCoverageScript())
-            .add(action.getCoverageDirectoryTreeArtifact())
             .add(action.getCoverageManifest())
             .addTransitive(action.getLcovMergerFilesToRun().build())
             .build(),

diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD b/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/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/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,32 @@ 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);

diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java
@@ -252,7 +252,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);

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/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);
 

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/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);
 

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/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);
 

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/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);
 

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -25,7 +25,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;
@@ -40,10 +39,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;
@@ -429,27 +426,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<>();

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java b/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);

diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
@@ -58,7 +58,8 @@ static SortedMap<PathFragment, HashCode> getWorkerFilesWithHashes(
     TreeMap<PathFragment, HashCode> workerFilesMap = new TreeMap<>();
 
     List<ActionInput> tools =
-        ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander);
+        ActionInputHelper.expandArtifacts(
+            spawn.getToolFiles(), artifactExpander, /* keepEmptyTreeArtifacts= */ false);
     for (ActionInput tool : tools) {
       workerFilesMap.put(
           tool.getExecPath(),

diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -192,8 +192,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);
@@ -250,7 +248,10 @@ 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();