Skip to content

Commit

Permalink
Remote: Fixed a bug that remote cache is missed due to executable bit…
Browse files Browse the repository at this point in the history
… is changed

Fixes bazelbuild#13262.

bazelbuild#12820 changed to set executable bit of input files based on its real value. However, this causes cache misses in `--remote_download_toplevel` mode since executable bit is changed after action execution by `ActionMetadataHandler#getMetadata`.

This change effectively rolls back bazelbuild#12820.

Closes bazelbuild#13276.

PiperOrigin-RevId: 367009617
  • Loading branch information
coeuvre authored and philwo committed Apr 19, 2021
1 parent a3a1763 commit d2b9428
Show file tree
Hide file tree
Showing 6 changed files with 93 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,37 @@ static class FileNode extends Node {
private final Digest digest;
private final boolean isExecutable;

FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
/**
* Create a FileNode with its executable bit set.
*
* <p>We always treat files as executable since Bazel will `chmod 555` on the output files of an
* action within ActionMetadataHandler#getMetadata after action execution if no metadata was
* injected. We can't use real executable bit of the file until this behaviour is changed. See
* https://github.com/bazelbuild/bazel/issues/13262 for more details.
*/
static FileNode createExecutable(String pathSegment, Path path, Digest digest) {
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true);
}

static FileNode createExecutable(String pathSegment, ByteString data, Digest digest) {
return new FileNode(pathSegment, data, digest, /* isExecutable= */ true);
}

private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
super(pathSegment);
this.path = Preconditions.checkNotNull(path, "path");
this.data = null;
this.digest = Preconditions.checkNotNull(digest, "digest");
this.isExecutable = isExecutable;
}

FileNode(String pathSegment, ByteString data, Digest digest) {
private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable) {
super(pathSegment);
this.path = null;
this.data = Preconditions.checkNotNull(data, "data");
;
this.digest = Preconditions.checkNotNull(digest, "digest");
this.isExecutable = false;
this.isExecutable = isExecutable;
}

Digest getDigest() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
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;
Expand Down Expand Up @@ -102,7 +101,7 @@ private static int buildFromPaths(
throw new IOException(String.format("Input '%s' is not a file.", input));
}
Digest d = digestUtil.compute(input);
currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable()));
currDir.addChild(FileNode.createExecutable(path.getBaseName(), input, d));
return 1;
});
}
Expand All @@ -128,7 +127,8 @@ private static int buildFromActionInputs(
if (input instanceof VirtualActionInput) {
VirtualActionInput virtualActionInput = (VirtualActionInput) input;
Digest d = digestUtil.compute(virtualActionInput);
currDir.addChild(new FileNode(path.getBaseName(), virtualActionInput.getBytes(), d));
currDir.addChild(
FileNode.createExecutable(path.getBaseName(), virtualActionInput.getBytes(), d));
return 1;
}

Expand All @@ -141,15 +141,7 @@ private static int buildFromActionInputs(
case REGULAR_FILE:
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);

boolean isExecutable;
if (metadata instanceof RemoteActionFileArtifactValue) {
isExecutable = ((RemoteActionFileArtifactValue) metadata).isExecutable();
} else {
isExecutable = inputPath.isExecutable();
}

currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable));
currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d));
return 1;

case DIRECTORY:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ public void virtualActionInputShouldWork() throws Exception {
assertThat(directoriesAtDepth(1, tree)).isEmpty();

FileNode expectedFooNode =
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
FileNode expectedBarNode =
new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
FileNode.createExecutable("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
}
Expand Down Expand Up @@ -117,19 +117,13 @@ public void directoryInputShouldBeExpanded() throws Exception {
assertThat(directoriesAtDepth(3, tree)).isEmpty();

FileNode expectedFooNode =
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
FileNode expectedBarNode =
new FileNode(
"bar.cc",
execRoot.getRelative(bar.getExecPath()),
digestUtil.computeAsUtf8("bar"),
false);
FileNode.createExecutable(
"bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"));
FileNode expectedBuzzNode =
new FileNode(
"buzz.cc",
execRoot.getRelative(buzz.getExecPath()),
digestUtil.computeAsUtf8("buzz"),
false);
FileNode.createExecutable(
"buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"));
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,12 @@ public void buildingATreeOfFilesShouldWork() throws Exception {
assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz");
assertThat(directoriesAtDepth(2, tree)).isEmpty();

FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false);
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false);
FileNode expectedFooNode =
FileNode.createExecutable("foo.cc", foo, digestUtil.computeAsUtf8("foo"));
FileNode expectedBarNode =
FileNode.createExecutable("bar.cc", bar, digestUtil.computeAsUtf8("bar"));
FileNode expectedBuzzNode =
new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false);
FileNode.createExecutable("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"));
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,13 @@ public void buildMerkleTree() throws IOException {

Directory fizzDir =
Directory.newBuilder()
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), false))
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), false))
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), true))
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), true))
.build();
Directory srcsDir =
Directory.newBuilder()
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), false))
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), false))
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), true))
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), true))
.addDirectories(
DirectoryNode.newBuilder().setName("fizz").setDigest(digestUtil.compute(fizzDir)))
.build();
Expand Down
64 changes: 54 additions & 10 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2194,26 +2194,70 @@ EOF
@local_foo//:all
}

function test_remote_input_files_executable_bit() {
# Test that input files uploaded to remote executor have the same executable bit with local files. #12818
function test_remote_cache_intermediate_outputs() {
# test that remote cache is hit when intermediate output is not executable
touch WORKSPACE
cat > BUILD <<'EOF'
genrule(
name = "dep",
srcs = [],
outs = ["dep"],
cmd = "echo 'dep' > $@",
)
genrule(
name = "test",
srcs = ["foo.txt", "bar.sh"],
outs = ["out.txt"],
cmd = "ls -l $(SRCS); touch $@",
srcs = [":dep"],
outs = ["out"],
cmd = "cat $(SRCS) > $@",
)
EOF
touch foo.txt bar.sh
chmod a+x bar.sh

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

bazel clean

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

expect_log "2 remote cache hit"
}

function test_remote_cache_intermediate_outputs_toplevel() {
# test that remote cache is hit when intermediate output is not executable in remote download toplevel mode
touch WORKSPACE
cat > BUILD <<'EOF'
genrule(
name = "dep",
srcs = [],
outs = ["dep"],
cmd = "echo 'dep' > $@",
)
genrule(
name = "test",
srcs = [":dep"],
outs = ["out"],
cmd = "cat $(SRCS) > $@",
)
EOF

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

bazel clean

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

expect_log "-rwxr--r-- .* bar.sh"
expect_log "-rw-r--r-- .* foo.txt"
expect_log "2 remote cache hit"
}

function test_exclusive_tag() {
Expand Down

0 comments on commit d2b9428

Please sign in to comment.