From ae79855a8bd4f38a77a8b681851897455716a045 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 1 May 2022 10:16:01 +0200 Subject: [PATCH] Create output directories with remote execution --- .../lib/remote/RemoteExecutionService.java | 30 +++++++++++++ .../bazel/remote/remote_execution_test.sh | 42 +++++++++++++++++++ .../build/remote/worker/ExecutionServer.java | 4 +- 3 files changed, 74 insertions(+), 2 deletions(-) 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 e274afcac6e410..8426369900f391 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 @@ -62,6 +62,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.eventbus.Subscribe; @@ -305,8 +306,26 @@ public boolean mayBeExecutedRemotely(Spawn spawn) { && Spawns.mayBeExecutedRemotely(spawn); } + private SortedMap buildOutputDirMap(Spawn spawn) { + ImmutableSortedMap.Builder outputDirMap = new ImmutableSortedMap.Builder<>( + PathFragment::compareTo); + for (ActionInput output : spawn.getOutputFiles()) { + if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { + outputDirMap.put( + PathFragment.create(remotePathResolver.getWorkingDirectory()) + .getRelative(output.getExecPath()), + output); + } + } + return outputDirMap.build(); + } + 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 +335,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); + new Throwable().printStackTrace(); + if (!outputDirMap.isEmpty()) { + ImmutableSortedMap.Builder builder = new ImmutableSortedMap.Builder<>( + PathFragment::compareTo); + builder.putAll(inputMap); + builder.putAll(outputDirMap); + inputMap = builder.build(); + } return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil); } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 34d5c3bc253e3e..57d3f0fc119f76 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3119,6 +3119,48 @@ EOF //pkg:a &>$TEST_log || fail "expected build to succeed with sibling repository layout" } +function test_create_tree_artifact_outputs() { + touch WORKSPACE + mkdir -p pkg + + cat > pkg/def.bzl <<'EOF' +def _r(ctx): + d = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [d], + command = "cd %s && pwd" % d.path, + ) + return [DefaultInfo(files = depset([d]))] + +r = rule(implementation = _r) +EOF + +cat > pkg/BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:a &>$TEST_log || fail "expected build to succeed" + + 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" + + 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" +} + # 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 af325e3b3b1a8b..14189f015f6f9b 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,8 +283,8 @@ 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.isFile()) { + throw new FileAlreadyExistsException("Existing file at output directory path: " + file); } file.getParentDirectory().createDirectoryAndParents(); outputs.add(file);