diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 62fecc7a92cee5..cc1727fe586be9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/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 buildOutputDirMap(Spawn spawn) { + TreeMap 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 outputDirMap = buildOutputDirMap(spawn); if (remoteOptions.remoteMerkleTreeCache) { MetadataProvider metadataProvider = context.getMetadataProvider(); ConcurrentLinkedQueue 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 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 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; } 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 index b7831a8136a7f6..bca6b54363c084 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD +++ b/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", 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 index c641f80accfe0c..f188b234ef18d6 100644 --- 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 @@ -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 tree) throws IOException { return build( - null, inputs, tree, (input, path, currDir) -> { @@ -126,7 +123,6 @@ private static int buildFromActionInputs( Map tree) throws IOException { return build( - metadataProvider, inputs, tree, (input, path, currDir) -> { @@ -182,7 +178,6 @@ private static int buildFromActionInputs( } private static int build( - @Nullable MetadataProvider metadataProvider, SortedMap inputs, Map tree, FileNodeVisitor fileNodeVisitor) @@ -200,24 +195,13 @@ private static 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); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 037a386469dda9..c215615c12255a 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/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. diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index c1e23f417af4b0..ffb7a61d750685 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/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);