Unify sandbox/remote handling of empty TreeArtifacts

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 an integration test for this previously untested case
and extracts the logic into SpawnInputExpander. It also updates some
comments that didn't mention TreeArtifact expansion even though it was
already performed before this commit.

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

@@ -103,12 +103,13 @@ 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) {
+      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 +119,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,31 @@ 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.
+   */
   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.
    */
   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 +1554,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,
+                    false);
             for (ActionInput input : expandedInputs) {
               addMapping(
                   inputMap,
@@ -222,7 +223,8 @@ private static void addInputs(
       NestedSet<? extends ActionInput> inputFiles,
       ArtifactExpander artifactExpander,
       PathFragment baseDirectory) {
-    List<ActionInput> inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander);
+    List<ActionInput> inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander,
+        true);
     for (ActionInput input : inputs) {
       addMapping(inputMap, input.getExecPath(), input, baseDirectory);
     }

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

@@ -487,27 +487,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,7 @@ 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, 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);
@@ -258,7 +256,8 @@ private WorkRequest createWorkRequest(
     }
 
     List<ActionInput> inputs =
-        ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander());
+        ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander(),
+            false);
 
     for (ActionInput input : inputs) {
       byte[] digestBytes = inputFileCache.getMetadata(input).getDigest();

src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java

@@ -24,14 +24,11 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
-import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.exec.BinTools;
-import com.google.devtools.build.lib.exec.util.SpawnBuilder;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
 import com.google.devtools.build.lib.testutil.Scratch;
@@ -66,9 +63,6 @@
 @RunWith(JUnit4.class)
 public class SandboxHelpersTest {
 
-  private static final ArtifactExpander EMPTY_EXPANDER = (ignored1, ignored2) -> {};
-  private static final Spawn SPAWN = new SpawnBuilder().build();
-
   private final Scratch scratch = new Scratch();
   private Path execRoot;
   @Nullable private ExecutorService executorToCleanup;
@@ -99,7 +93,7 @@ public void processInputFiles_materializesParamFile() throws Exception {
             UTF_8);
 
     SandboxInputs inputs =
-        sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+        sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot);
 
     assertThat(inputs.getFiles())
         .containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile"));
@@ -119,7 +113,7 @@ public void processInputFiles_materializesBinToolsFile() throws Exception {
             PathFragment.create("_bin/say_hello"));
 
     SandboxInputs inputs =
-        sandboxHelpers.processInputFiles(inputMap(tool), SPAWN, EMPTY_EXPANDER, execRoot);
+        sandboxHelpers.processInputFiles(inputMap(tool), execRoot);
 
     assertThat(inputs.getFiles())
         .containsExactly(
@@ -143,7 +137,7 @@ public void processInputFiles_delayVirtualInputMaterialization_doesNotStoreVirtu
             UTF_8);
 
     SandboxInputs inputs =
-        sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+        sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot);
 
     assertThat(inputs.getFiles()).isEmpty();
     assertThat(inputs.getSymlinks()).isEmpty();
@@ -165,7 +159,7 @@ public void sandboxInputMaterializeVirtualInputs_delayMaterialization_writesCorr
             scratch.file("tool", "tool_code"), PathFragment.create("tools/tool"));
     SandboxInputs inputs =
         sandboxHelpers.processInputFiles(
-            inputMap(paramFile, tool), SPAWN, EMPTY_EXPANDER, execRoot);
+            inputMap(paramFile, tool), execRoot);
 
     inputs.materializeVirtualInputs(scratch.dir("/sandbox"));
 
@@ -213,13 +207,13 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc
             () -> {
               try {
                 sandboxHelpers.processInputFiles(
-                    inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot);
+                    inputMap(input), customExecRoot);
                 finishProcessingSemaphore.release();
               } catch (IOException e) {
                 throw new IllegalArgumentException(e);
               }
             });
-    sandboxHelpers.processInputFiles(inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot);
+    sandboxHelpers.processInputFiles(inputMap(input), customExecRoot);
     finishProcessingSemaphore.release();
     future.get();
 

src/test/shell/bazel/bazel_sandboxing_test.sh

@@ -794,7 +794,7 @@ EOF
 }
 
 # regression test for https://github.com/bazelbuild/bazel/issues/6262
-function test_create_tree_artifact_inputs() {
+function test_create_tree_artifact_outputs() {
   create_workspace_with_default_repos WORKSPACE
 
   cat > def.bzl <<'EOF'
@@ -818,6 +818,36 @@ EOF
   bazel build --test_output=streamed :a &>$TEST_log || fail "expected build to succeed"
 }
 
+function test_create_tree_artifact_inputs() {
+  create_workspace_with_default_repos WORKSPACE
+
+  cat > def.bzl <<'EOF'
+def _r(ctx):
+    empty_d = ctx.actions.declare_directory("%s_empty_dir" % ctx.label.name)
+    ctx.actions.run_shell(
+        outputs = [empty_d],
+        command = "mkdir -p %s" % empty_d.path,
+    )
+    f = ctx.actions.declare_file("%s" % ctx.label.name)
+    ctx.actions.run_shell(
+        inputs = [empty_d],
+        outputs = [f],
+        command = "cd %s && pwd > %s" % (empty_d.path, f.path),
+    )
+    return [DefaultInfo(files = depset([f]))]
+
+r = rule(implementation = _r)
+EOF
+
+cat > BUILD <<'EOF'
+load(":def.bzl", "r")
+
+r(name = "a")
+EOF
+
+  bazel build :a &>$TEST_log || fail "expected build to succeed"
+}
+
 # The test shouldn't fail if the environment doesn't support running it.
 check_sandbox_allowed || exit 0