Skip to content

Commit 1404651

Browse files
fmeumckolli5
fmeum
and
ckolli5
authored
Create output directories for remote execution (bazelbuild#15818)
By explicitly declaring output directories as inputs to a remote action, this commit ensures that these directories are created by the remote executor prior to the execution of the action. This brings the behavior of remote execution regarding tree artifacts in line with that of local or sandboxed execution. Getting the tests to pass requires modifying a check in Bazel's own remote worker implementation: Previously, the worker explicitly verified that output directories don't exist after the inputs have been staged. This behavior is not backed by the spec and has thus been modified: Now, it is only checked that the output directories either don't exist or are directories. Fixes bazelbuild#6393 Closes bazelbuild#15366. PiperOrigin-RevId: 447451303 Co-authored-by: Chenchu K <[email protected]>
1 parent 64571a4 commit 1404651

File tree

5 files changed

+126
-27
lines changed

5 files changed

+126
-27
lines changed

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

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@
139139
import java.util.Objects;
140140
import java.util.Set;
141141
import java.util.SortedMap;
142+
import java.util.TreeMap;
142143
import java.util.TreeSet;
143144
import java.util.concurrent.ConcurrentLinkedQueue;
144145
import java.util.concurrent.Executor;
@@ -305,8 +306,25 @@ public boolean mayBeExecutedRemotely(Spawn spawn) {
305306
&& Spawns.mayBeExecutedRemotely(spawn);
306307
}
307308

309+
private SortedMap<PathFragment, ActionInput> buildOutputDirMap(Spawn spawn) {
310+
TreeMap<PathFragment, ActionInput> outputDirMap = new TreeMap<>();
311+
for (ActionInput output : spawn.getOutputFiles()) {
312+
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
313+
outputDirMap.put(
314+
PathFragment.create(remotePathResolver.getWorkingDirectory())
315+
.getRelative(remotePathResolver.localPathToOutputPath(output.getExecPath())),
316+
output);
317+
}
318+
}
319+
return outputDirMap;
320+
}
321+
308322
private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext context)
309323
throws IOException, ForbiddenActionInputException {
324+
// Add output directories to inputs so that they are created as empty directories by the
325+
// executor. The spec only requires the executor to create the parent directory of an output
326+
// directory, which differs from the behavior of both local and sandboxed execution.
327+
SortedMap<PathFragment, ActionInput> outputDirMap = buildOutputDirMap(spawn);
310328
if (remoteOptions.remoteMerkleTreeCache) {
311329
MetadataProvider metadataProvider = context.getMetadataProvider();
312330
ConcurrentLinkedQueue<MerkleTree> subMerkleTrees = new ConcurrentLinkedQueue<>();
@@ -316,9 +334,20 @@ private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext conte
316334
(Object nodeKey, InputWalker walker) -> {
317335
subMerkleTrees.add(buildMerkleTreeVisitor(nodeKey, walker, metadataProvider));
318336
});
337+
if (!outputDirMap.isEmpty()) {
338+
subMerkleTrees.add(MerkleTree.build(outputDirMap, metadataProvider, execRoot, digestUtil));
339+
}
319340
return MerkleTree.merge(subMerkleTrees, digestUtil);
320341
} else {
321342
SortedMap<PathFragment, ActionInput> inputMap = remotePathResolver.getInputMapping(context);
343+
if (!outputDirMap.isEmpty()) {
344+
// The map returned by getInputMapping is mutable, but must not be mutated here as it is
345+
// shared with all other strategies.
346+
SortedMap<PathFragment, ActionInput> newInputMap = new TreeMap<>();
347+
newInputMap.putAll(inputMap);
348+
newInputMap.putAll(outputDirMap);
349+
inputMap = newInputMap;
350+
}
322351
return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
323352
}
324353
}
@@ -937,9 +966,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
937966
// Check that all mandatory outputs are created.
938967
for (ActionInput output : action.getSpawn().getOutputFiles()) {
939968
if (action.getSpawn().isMandatoryOutput(output)) {
940-
// Don't check output that is tree artifact since spawn could generate nothing under that
941-
// directory. Remote server typically doesn't create directory ahead of time resulting in
942-
// empty tree artifact missing from action cache entry.
969+
// In the past, remote execution did not create output directories if the action didn't do
970+
// this explicitly. This check only remains so that old remote cache entries that do not
971+
// include empty output directories remain valid.
943972
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
944973
continue;
945974
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ java_library(
2121
"//src/main/java/com/google/devtools/build/lib/profiler",
2222
"//src/main/java/com/google/devtools/build/lib/remote/util",
2323
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
24+
"//src/main/java/com/google/devtools/build/lib/util:string",
2425
"//src/main/java/com/google/devtools/build/lib/vfs",
2526
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
2627
"//third_party:guava",

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

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
2525
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
2626
import com.google.devtools.build.lib.remote.util.DigestUtil;
27-
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
2827
import com.google.devtools.build.lib.vfs.Dirent;
2928
import com.google.devtools.build.lib.vfs.Path;
3029
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -35,7 +34,6 @@
3534
import java.util.Map;
3635
import java.util.SortedMap;
3736
import java.util.TreeMap;
38-
import javax.annotation.Nullable;
3937

4038
/** Builder for directory trees. */
4139
class DirectoryTreeBuilder {
@@ -97,7 +95,6 @@ private static int buildFromPaths(
9795
Map<PathFragment, DirectoryNode> tree)
9896
throws IOException {
9997
return build(
100-
null,
10198
inputs,
10299
tree,
103100
(input, path, currDir) -> {
@@ -126,7 +123,6 @@ private static int buildFromActionInputs(
126123
Map<PathFragment, DirectoryNode> tree)
127124
throws IOException {
128125
return build(
129-
metadataProvider,
130126
inputs,
131127
tree,
132128
(input, path, currDir) -> {
@@ -182,7 +178,6 @@ private static int buildFromActionInputs(
182178
}
183179

184180
private static <T> int build(
185-
@Nullable MetadataProvider metadataProvider,
186181
SortedMap<PathFragment, T> inputs,
187182
Map<PathFragment, DirectoryNode> tree,
188183
FileNodeVisitor<T> fileNodeVisitor)
@@ -200,24 +195,13 @@ private static <T> int build(
200195
T input = e.getValue();
201196

202197
if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) {
203-
DerivedArtifact artifact = (DerivedArtifact) input;
204-
// MetadataProvider is provided by all callers for which T is a superclass of
205-
// DerivedArtifact.
206-
Preconditions.checkNotNull(metadataProvider);
207-
FileArtifactValue metadata =
208-
Preconditions.checkNotNull(
209-
metadataProvider.getMetadata(artifact),
210-
"missing metadata for '%s'",
211-
artifact.getExecPathString());
212-
Preconditions.checkState(
213-
metadata.equals(TreeArtifactValue.empty().getMetadata()),
214-
"Encountered non-empty TreeArtifact '%s' with metadata '%s', which should have"
215-
+ " been expanded by SpawnInputExpander. This is a bug.",
216-
path,
217-
metadata);
218-
// Create an empty directory and its parent directories but don't visit the TreeArtifact
219-
// input itself: A TreeArtifact's metadata has type REGULAR_FILE, not DIRECTORY, and would
220-
// thus lead to an empty file being created in the buildFromActionInputs visitor.
198+
// SpawnInputExpander has already expanded non-empty tree artifacts into a collection of
199+
// TreeFileArtifacts. Thus, at this point, tree artifacts represent empty directories, which
200+
// we create together with their parents.
201+
// Note: This also handles output directories of actions, which are explicitly included as
202+
// inputs so that they are created by the executor before the action executes. Since such a
203+
// directory must remain writeable, MetadataProvider#getMetadata must not be called on the
204+
// tree artifact here as it would have the side effect of making it read only.
221205
DirectoryNode emptyDir = new DirectoryNode(path.getBaseName());
222206
tree.put(path, emptyDir);
223207
createParentDirectoriesIfNotExist(path, emptyDir, tree);

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3052,6 +3052,85 @@ function test_empty_tree_artifact_as_inputs_remote_cache() {
30523052
expect_log "remote cache hit"
30533053
}
30543054

3055+
function generate_tree_artifact_output() {
3056+
touch WORKSPACE
3057+
mkdir -p pkg
3058+
3059+
cat > pkg/def.bzl <<'EOF'
3060+
def _r(ctx):
3061+
empty_dir = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name)
3062+
ctx.actions.run_shell(
3063+
outputs = [empty_dir],
3064+
command = "cd %s && pwd" % empty_dir.path,
3065+
)
3066+
non_empty_dir = ctx.actions.declare_directory("%s/non_empty_dir" % ctx.label.name)
3067+
ctx.actions.run_shell(
3068+
outputs = [non_empty_dir],
3069+
command = "cd %s && pwd && touch out" % non_empty_dir.path,
3070+
)
3071+
return [DefaultInfo(files = depset([empty_dir, non_empty_dir]))]
3072+
3073+
r = rule(implementation = _r)
3074+
EOF
3075+
3076+
cat > pkg/BUILD <<'EOF'
3077+
load(":def.bzl", "r")
3078+
3079+
r(name = "a")
3080+
EOF
3081+
}
3082+
3083+
function test_create_tree_artifact_outputs() {
3084+
# Test that if a tree artifact is declared as an input, then the corresponding
3085+
# empty directory is created before the action executes remotely.
3086+
generate_tree_artifact_output
3087+
3088+
bazel build \
3089+
--spawn_strategy=remote \
3090+
--remote_executor=grpc://localhost:${worker_port} \
3091+
//pkg:a &>$TEST_log || fail "expected build to succeed"
3092+
[[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
3093+
[[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
3094+
3095+
bazel clean --expunge
3096+
bazel build \
3097+
--spawn_strategy=remote \
3098+
--remote_executor=grpc://localhost:${worker_port} \
3099+
--experimental_remote_merkle_tree_cache \
3100+
//pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree cache"
3101+
[[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
3102+
[[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
3103+
3104+
bazel clean --expunge
3105+
bazel build \
3106+
--spawn_strategy=remote \
3107+
--remote_executor=grpc://localhost:${worker_port} \
3108+
--experimental_sibling_repository_layout \
3109+
//pkg:a &>$TEST_log || fail "expected build to succeed with sibling repository layout"
3110+
[[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
3111+
[[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
3112+
}
3113+
3114+
function test_create_tree_artifact_outputs_remote_cache() {
3115+
# Test that implicitly created empty directories corresponding to empty tree
3116+
# artifacts outputs are correctly cached in the remote cache.
3117+
generate_tree_artifact_output
3118+
3119+
bazel build \
3120+
--remote_cache=grpc://localhost:${worker_port} \
3121+
//pkg:a &>$TEST_log || fail "expected build to succeed"
3122+
3123+
bazel clean
3124+
3125+
bazel build \
3126+
--remote_cache=grpc://localhost:${worker_port} \
3127+
//pkg:a &>$TEST_log || fail "expected build to succeed"
3128+
3129+
expect_log "2 remote cache hit"
3130+
[[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file"
3131+
[[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
3132+
}
3133+
30553134
# Runs coverage with `cc_test` and RE then checks the coverage file is returned.
30563135
# Older versions of gcov are not supported with bazel coverage and so will be skipped.
30573136
# 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‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,13 @@ private ActionResult execute(
283283
for (String output : command.getOutputDirectoriesList()) {
284284
Path file = workingDirectory.getRelative(output);
285285
if (file.exists()) {
286-
throw new FileAlreadyExistsException("Output directory/file already exists: " + file);
286+
if (!file.isDirectory()) {
287+
throw new FileAlreadyExistsException(
288+
"Non-directory exists at output directory path: " + file);
289+
} else if (!file.getDirectoryEntries().isEmpty()) {
290+
throw new FileAlreadyExistsException(
291+
"Non-empty directory exists at output directory path: " + file);
292+
}
287293
}
288294
FileSystemUtils.createDirectoryAndParents(file.getParentDirectory());
289295
outputs.add(file);

0 commit comments

Comments
 (0)