Create output directories for remote execution

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

@@ -139,6 +139,7 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.SortedMap;
+import java.util.TreeMap;
 import java.util.TreeSet;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.Executor;
@@ -305,8 +306,25 @@ public boolean mayBeExecutedRemotely(Spawn spawn) {
         && Spawns.mayBeExecutedRemotely(spawn);
   }
 
+  private SortedMap<PathFragment, ActionInput> buildOutputDirMap(Spawn spawn) {
+    TreeMap<PathFragment, ActionInput> outputDirMap = new TreeMap<>();
+    for (ActionInput output : spawn.getOutputFiles()) {
+      if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
+        outputDirMap.put(
+            PathFragment.create(remotePathResolver.getWorkingDirectory())
+                .getRelative(remotePathResolver.localPathToOutputPath(output.getExecPath())),
+            output);
+      }
+    }
+    return outputDirMap;
+  }
+
   private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext context)
       throws IOException, ForbiddenActionInputException {
+    // Add output directories to inputs so that they are created as empty directories by the
+    // executor. The spec only requires the executor to create the parent directory of an output
+    // directory, which differs from the behavior of both local and sandboxed execution.
+    SortedMap<PathFragment, ActionInput> outputDirMap = buildOutputDirMap(spawn);
     if (remoteOptions.remoteMerkleTreeCache) {
       MetadataProvider metadataProvider = context.getMetadataProvider();
       ConcurrentLinkedQueue<MerkleTree> subMerkleTrees = new ConcurrentLinkedQueue<>();
@@ -316,9 +334,20 @@ private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext conte
           (Object nodeKey, InputWalker walker) -> {
             subMerkleTrees.add(buildMerkleTreeVisitor(nodeKey, walker, metadataProvider));
           });
+      if (!outputDirMap.isEmpty()) {
+        subMerkleTrees.add(MerkleTree.build(outputDirMap, metadataProvider, execRoot, digestUtil));
+      }
       return MerkleTree.merge(subMerkleTrees, digestUtil);
     } else {
       SortedMap<PathFragment, ActionInput> inputMap = remotePathResolver.getInputMapping(context);
+      if (!outputDirMap.isEmpty()) {
+        // The map returned by getInputMapping is mutable, but must not be mutated here as it is
+        // shared with all other strategies.
+        SortedMap<PathFragment, ActionInput> newInputMap = new TreeMap<>();
+        newInputMap.putAll(inputMap);
+        newInputMap.putAll(outputDirMap);
+        inputMap = newInputMap;
+      }
       return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
     }
   }
@@ -937,9 +966,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       // Check that all mandatory outputs are created.
       for (ActionInput output : action.getSpawn().getOutputFiles()) {
         if (action.getSpawn().isMandatoryOutput(output)) {
-          // Don't check output that is tree artifact since spawn could generate nothing under that
-          // directory. Remote server typically doesn't create directory ahead of time resulting in
-          // empty tree artifact missing from action cache entry.
+          // In the past, remote execution did not create output directories if the action didn't do
+          // this explicitly. This check only remains so that old remote cache entries that do not
+          // include empty output directories remain valid.
           if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
             continue;
           }

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

@@ -21,6 +21,7 @@ java_library(
         "//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",
         "//third_party:guava",

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

@@ -24,7 +24,6 @@
 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;
@@ -35,7 +34,6 @@
 import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
-import javax.annotation.Nullable;
 
 /** Builder for directory trees. */
 class DirectoryTreeBuilder {
@@ -97,7 +95,6 @@ private static int buildFromPaths(
       Map<PathFragment, DirectoryNode> tree)
       throws IOException {
     return build(
-        null,
         inputs,
         tree,
         (input, path, currDir) -> {
@@ -126,7 +123,6 @@ private static int buildFromActionInputs(
       Map<PathFragment, DirectoryNode> tree)
       throws IOException {
     return build(
-        metadataProvider,
         inputs,
         tree,
         (input, path, currDir) -> {
@@ -182,7 +178,6 @@ private static int buildFromActionInputs(
   }
 
   private static <T> int build(
-      @Nullable MetadataProvider metadataProvider,
       SortedMap<PathFragment, T> inputs,
       Map<PathFragment, DirectoryNode> tree,
       FileNodeVisitor<T> fileNodeVisitor)
@@ -200,24 +195,13 @@ private static <T> int build(
       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.
+        // SpawnInputExpander has already expanded non-empty tree artifacts into a collection of
+        // TreeFileArtifacts. Thus, at this point, tree artifacts represent empty directories, which
+        // we create together with their parents.
+        // Note: This also handles output directories of actions, which are explicitly included as
+        // inputs so that they are created by the executor before the action executes. Since such a
+        // directory must remain writeable, MetadataProvider#getMetadata must not be called on the
+        // tree artifact here as it would have the side effect of making it read only.
         DirectoryNode emptyDir = new DirectoryNode(path.getBaseName());
         tree.put(path, emptyDir);
         createParentDirectoriesIfNotExist(path, emptyDir, tree);

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -3052,6 +3052,85 @@ function test_empty_tree_artifact_as_inputs_remote_cache() {
   expect_log "remote cache hit"
 }
 
+function generate_tree_artifact_output() {
+  touch WORKSPACE
+  mkdir -p pkg
+
+  cat > pkg/def.bzl <<'EOF'
+def _r(ctx):
+    empty_dir = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name)
+    ctx.actions.run_shell(
+        outputs = [empty_dir],
+        command = "cd %s && pwd" % empty_dir.path,
+    )
+    non_empty_dir = ctx.actions.declare_directory("%s/non_empty_dir" % ctx.label.name)
+    ctx.actions.run_shell(
+        outputs = [non_empty_dir],
+        command = "cd %s && pwd && touch out" % non_empty_dir.path,
+    )
+    return [DefaultInfo(files = depset([empty_dir, non_empty_dir]))]
+
+r = rule(implementation = _r)
+EOF
+
+cat > pkg/BUILD <<'EOF'
+load(":def.bzl", "r")
+
+r(name = "a")
+EOF
+}
+
+function test_create_tree_artifact_outputs() {
+  # Test that if a tree artifact is declared as an input, then the corresponding
+  # empty directory is created before the action executes remotely.
+  generate_tree_artifact_output
+
+  bazel build \
+    --spawn_strategy=remote \
+    --remote_executor=grpc://localhost:${worker_port} \
+    //pkg:a &>$TEST_log || fail "expected build to succeed"
+  [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
+  [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
+
+  bazel clean --expunge
+  bazel build \
+    --spawn_strategy=remote \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_remote_merkle_tree_cache \
+    //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree cache"
+  [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
+  [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
+
+  bazel clean --expunge
+  bazel build \
+    --spawn_strategy=remote \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_sibling_repository_layout \
+    //pkg:a &>$TEST_log || fail "expected build to succeed with sibling repository layout"
+  [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
+  [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
+}
+
+function test_create_tree_artifact_outputs_remote_cache() {
+  # Test that implicitly created empty directories corresponding to empty tree
+  # artifacts outputs are correctly cached in the remote cache.
+  generate_tree_artifact_output
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //pkg:a &>$TEST_log || fail "expected build to succeed"
+
+  bazel clean
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //pkg:a &>$TEST_log || fail "expected build to succeed"
+
+  expect_log "2 remote cache hit"
+  [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
+  [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
+}
+
 # Runs coverage with `cc_test` and RE then checks the coverage file is returned.
 # Older versions of gcov are not supported with bazel coverage and so will be skipped.
 # See the above `test_java_rbe_coverage_produces_report` for more information.

src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java

@@ -283,7 +283,13 @@ private ActionResult execute(
     for (String output : command.getOutputDirectoriesList()) {
       Path file = workingDirectory.getRelative(output);
       if (file.exists()) {
-        throw new FileAlreadyExistsException("Output directory/file already exists: " + file);
+        if (!file.isDirectory()) {
+          throw new FileAlreadyExistsException(
+              "Non-directory exists at output directory path: " + file);
+        } else if (!file.getDirectoryEntries().isEmpty()) {
+          throw new FileAlreadyExistsException(
+              "Non-empty directory exists at output directory path: " + file);
+        }
       }
       FileSystemUtils.createDirectoryAndParents(file.getParentDirectory());
       outputs.add(file);