diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java index 19d4b33c4e1ccb..ed05554945a260 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java @@ -74,7 +74,23 @@ 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. + * + *

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; @@ -82,12 +98,12 @@ static class FileNode extends Node { 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() { 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 4503f741dbbfae..fef3fad557e4d8 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 @@ -102,7 +102,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; }); } @@ -128,7 +128,7 @@ 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; } @@ -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: diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java index c511973859a570..7305ccbc768bc9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java @@ -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); } @@ -117,19 +117,17 @@ 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( + FileNode.createExecutable( "bar.cc", execRoot.getRelative(bar.getExecPath()), - digestUtil.computeAsUtf8("bar"), - false); + digestUtil.computeAsUtf8("bar")); FileNode expectedBuzzNode = - new FileNode( + FileNode.createExecutable( "buzz.cc", execRoot.getRelative(buzz.getExecPath()), - digestUtil.computeAsUtf8("buzz"), - false); + digestUtil.computeAsUtf8("buzz")); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode); assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode); diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java index e4be7d41edbed3..c7abbb726232d7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java @@ -75,10 +75,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); diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java index 923f075c1b2faf..17d714828e05eb 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java @@ -88,13 +88,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(); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index dce00f2b848d26..1d9f4faed7537d 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -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() {