WIP: Expand empty tree artifacts to marker file

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

@@ -103,9 +103,9 @@ 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>Non-middleman, non-tree artifacts are returned untouched.
    */
   public static List<ActionInput> expandArtifacts(
       NestedSet<? extends ActionInput> inputs, ArtifactExpander artifactExpander) {

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

@@ -774,6 +774,10 @@ public boolean isChildOfDeclaredDirectory() {
     return false;
   }
 
+  public boolean isEmptyDirectoryMarker() {
+    return false;
+  }
+
   /**
    * Returns whether the artifact represents a Fileset.
    *
@@ -1292,6 +1296,10 @@ public ArchivedTreeArtifact deserialize(
    * </ol>
    */
   public static final class TreeFileArtifact extends DerivedArtifact {
+
+    private static final PathFragment EMPTY_DIRECTORY_MARKER = PathFragment.createAlreadyNormalized(
+        "BAZEL_TREE_FILE_ARTIFACT_EMPTY_DIRECTORY_MARKER");
+
     private final SpecialArtifact parent;
     private final PathFragment parentRelativePath;
 
@@ -1329,6 +1337,10 @@ public static TreeFileArtifact createTreeOutput(
       return createTreeOutput(parent, PathFragment.create(parentRelativePath));
     }
 
+    public static TreeFileArtifact createEmptyDirectoryMarker(SpecialArtifact parent) {
+      return createTreeOutput(parent, EMPTY_DIRECTORY_MARKER);
+    }
+
     /**
      * Creates a {@link TreeFileArtifact} representing the output of an action generated dynamically
      * by an {@link ActionTemplate} during the execution phase.
@@ -1392,6 +1404,11 @@ public boolean isChildOfDeclaredDirectory() {
       return !isActionTemplateExpansionKey(getArtifactOwner());
     }
 
+    @Override
+    public boolean isEmptyDirectoryMarker() {
+      return parentRelativePath == EMPTY_DIRECTORY_MARKER;
+    }
+
     private static boolean isActionTemplateExpansionKey(ActionLookupKey key) {
       return SkyFunctions.ACTION_TEMPLATE_EXPANSION.equals(key.functionName());
     }
@@ -1513,14 +1530,17 @@ public static String joinExecPaths(String delimiter, Iterable<Artifact> artifact
   }
 
   /**
-   * Renders a collection of artifacts as root-relative paths and joins
-   * them into a single string. Middleman artifacts are ignored by this method.
+   * Renders a collection of artifacts as root-relative paths and joins them into a single string.
+   * Middleman artifacts are ignored by this method.
    */
   public static String joinRootRelativePaths(String delimiter, Iterable<Artifact> artifacts) {
     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.
+   */
   static void addExpandedArtifacts(
       Iterable<Artifact> artifacts,
       Collection<? super Artifact> output,
@@ -1529,9 +1549,8 @@ static void addExpandedArtifacts(
   }
 
   /**
-   * 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.
    */
   private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifacts,
                                                Collection<? super E> output,
@@ -1556,6 +1575,10 @@ private static <E> void expandArtifact(Artifact middleman,
     for (Artifact artifact : artifacts) {
       output.add(outputFormatter.apply(artifact));
     }
+    if (middleman.isTreeArtifact() && artifacts.isEmpty()) {
+      output.add(outputFormatter.apply(TreeFileArtifact.createEmptyDirectoryMarker(
+          (SpecialArtifact) middleman)));
+    }
   }
 
   /**

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

@@ -19,6 +19,7 @@
 import com.google.common.hash.HashCode;
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
@@ -101,6 +102,9 @@ public void logSpawn(
         if (input instanceof VirtualActionInput.EmptyActionInput) {
           continue;
         }
+        if (input instanceof Artifact && ((Artifact) input).isEmptyDirectoryMarker()) {
+          continue;
+        }
         Path inputPath = execRoot.getRelative(input.getExecPathString());
         if (inputPath.isDirectory()) {
           listDirectoryContents(inputPath, builder::addInputs, metadataProvider);

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/RemoteActionInputFetcher.java

@@ -27,6 +27,7 @@
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
+import com.google.devtools.build.lib.actions.Artifact;
 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;
@@ -132,7 +133,7 @@ private Single<TransferResult> downloadOneInput(
                   SandboxHelpers.atomicallyWriteVirtualInput(
                       virtualActionInput, outputPath, ".remote");
                 }
-              } else {
+              } else if (!(input instanceof Artifact) || !((Artifact) input).isEmptyDirectoryMarker()) {
                 FileArtifactValue metadata = metadataProvider.getMetadata(input);
                 if (metadata != null && metadata.isRemote()) {
                   Path path = execRoot.getRelative(input.getExecPath());

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java

@@ -21,6 +21,7 @@
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Throwables;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
@@ -203,6 +204,9 @@ private void checkForConcurrentModifications()
             if (input instanceof VirtualActionInput) {
               continue;
             }
+            if (input instanceof Artifact && ((Artifact) input).isEmptyDirectoryMarker()) {
+              continue;
+            }
             FileArtifactValue metadata = context.getMetadataProvider().getMetadata(input);
             Path path = execRoot.getRelative(input.getExecPath());
             if (metadata.wasModifiedSinceDigest(path)) {

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

@@ -30,6 +30,7 @@
 import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
@@ -543,6 +544,9 @@ private Map<Path, Long> getInputCtimes(SortedMap<PathFragment, ActionInput> inpu
       if (input instanceof VirtualActionInput) {
         continue;
       }
+      if (input instanceof Artifact && ((Artifact) input).isEmptyDirectoryMarker()) {
+        continue;
+      }
       Path path = execRoot.getRelative(input.getExecPathString());
       try {
         ctimes.put(path, path.stat().getLastChangeTime());

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",

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

@@ -17,6 +17,7 @@
 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;
 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;
@@ -133,6 +134,8 @@ private static int buildFromActionInputs(
                     FileNode.createExecutable(
                         path.getBaseName(), virtualActionInput.getBytes(), d));
             return childAdded ? 1 : 0;
+          } else if (input instanceof Artifact && ((Artifact) input).isEmptyDirectoryMarker()) {
+            return 0;
           }
 
           FileArtifactValue metadata =

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);

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

@@ -20,6 +20,7 @@
 import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -184,8 +185,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);
 
@@ -389,6 +388,9 @@ private void checkForConcurrentModifications(SpawnExecutionContext context)
       if (input instanceof VirtualActionInput) {
         continue;
       }
+      if (input instanceof Artifact && ((Artifact) input).isEmptyDirectoryMarker()) {
+        continue;
+      }
 
       FileArtifactValue metadata = context.getMetadataProvider().getMetadata(input);
       Path path = execRoot.getRelative(input.getExecPath());

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<>();
@@ -516,6 +494,10 @@ public SandboxInputs processInputFiles(
       PathFragment pathFragment = e.getKey();
       ActionInput actionInput = e.getValue();
 
+      if (actionInput instanceof Artifact && ((Artifact) actionInput).isEmptyDirectoryMarker()) {
+        continue;
+      }
+
       // TODO(b/150963503): Make delayVirtualInputMaterialization the default and remove the
       // alternate code path.
       if (delayVirtualInputMaterialization) {

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/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);