From d2b942879471786e82f1c96eea8722bbe7919fc1 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 6 Apr 2021 08:22:57 -0700 Subject: [PATCH] Remote: Fixed a bug that remote cache is missed due to executable bit is changed Fixes https://github.com/bazelbuild/bazel/issues/13262. https://github.com/bazelbuild/bazel/pull/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 https://github.com/bazelbuild/bazel/pull/12820. Closes #13276. PiperOrigin-RevId: 367009617 --- .../lib/remote/merkletree/DirectoryTree.java | 22 ++++++- .../merkletree/DirectoryTreeBuilder.java | 16 ++--- .../ActionInputDirectoryTreeTest.java | 20 ++---- .../remote/merkletree/DirectoryTreeTest.java | 8 ++- .../lib/remote/merkletree/MerkleTreeTest.java | 8 +-- .../bazel/remote/remote_execution_test.sh | 64 ++++++++++++++++--- 6 files changed, 93 insertions(+), 45 deletions(-) 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 ebef7bff80446e..c31e9b40470c42 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,13 +98,13 @@ 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 80a34af8ab15db..2522bd3fb90d50 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 @@ -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; @@ -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; }); } @@ -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; } @@ -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..fcca592bb49bc1 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,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); 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 54c6a5c402c46b..01a6e3daff5691 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 @@ -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); 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 fbb1577c44af6e..fb0e150e052551 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 @@ -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(); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 0f177dfb485183..51fc9261b4630b 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() {