Skip to content

Commit

Permalink
Remote: Don't check TreeArtifact output (#15085)
Browse files Browse the repository at this point in the history
With a151116, we ignore action cache entry when checking the remote cache if a mandatory output of the Spawn is missing.

However, this breaks Spawns that declare a directory output but don't generate anything under it. Since remote servers typically don't create directory ahead of time, and if the action itself doesn't create files under the directory, that directory will be eliminated from the action cache entry.

This PR fixes that by not checking TreeArtifact output.

Closes #15077.

PiperOrigin-RevId: 436163970
(cherry picked from commit f585783)

Co-authored-by: Chi Wang <[email protected]>
  • Loading branch information
brentleyjones and coeuvre authored Mar 21, 2022
1 parent 6d26dc7 commit 376cd47
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
// Check that all mandatory outputs are created.
for (ActionInput output : action.spawn.getOutputFiles()) {
if (action.spawn.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.
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
continue;
}

Path localPath = execRoot.getRelative(output.getExecPath());
if (!metadata.files.containsKey(localPath)
&& !metadata.directories.containsKey(localPath)
Expand Down
57 changes: 57 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,63 @@ EOF
|| fail "Failed to run //a:starlark_output_dir_test with remote execution"
}

function generate_empty_treeartifact_build() {
mkdir -p a
cat > a/BUILD <<'EOF'
load(":output_dir.bzl", "gen_output_dir")
gen_output_dir(
name = "output-dir",
outdir = "dir",
)
EOF
cat > a/output_dir.bzl <<'EOF'
def _gen_output_dir_impl(ctx):
output_dir = ctx.actions.declare_directory(ctx.attr.outdir)
ctx.actions.run_shell(
outputs = [output_dir],
inputs = [],
command = "",
arguments = [output_dir.path],
)
return [
DefaultInfo(files = depset(direct = [output_dir])),
]
gen_output_dir = rule(
implementation = _gen_output_dir_impl,
attrs = {
"outdir": attr.string(mandatory = True),
},
)
EOF
}

function test_empty_treeartifact_works_with_remote_execution() {
# Test that empty tree artifact works with remote execution
generate_empty_treeartifact_build

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
//a:output-dir >& $TEST_log || fail "Failed to build"
}

function test_empty_treeartifact_works_with_remote_cache() {
# Test that empty tree artifact works with remote cache
generate_empty_treeartifact_build

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
//a:output-dir >& $TEST_log || fail "Failed to build"

bazel clean

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
//a:output-dir >& $TEST_log || fail "Failed to build"

expect_log "remote cache hit"
}

function test_downloads_minimal() {
# Test that genrule outputs are not downloaded when using
# --remote_download_minimal
Expand Down

0 comments on commit 376cd47

Please sign in to comment.