Skip to content

Commit d2b9428

Browse files
coeuvrephilwo
coeuvre
authored and
philwo
committed
Remote: Fixed a bug that remote cache is missed due to executable bit is changed
Fixes #13262. #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 #12820. Closes #13276. PiperOrigin-RevId: 367009617
1 parent a3a1763 commit d2b9428

File tree

6 files changed

+93
-45
lines changed

6 files changed

+93
-45
lines changed

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,21 +74,37 @@ static class FileNode extends Node {
7474
private final Digest digest;
7575
private final boolean isExecutable;
7676

77-
FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
77+
/**
78+
* Create a FileNode with its executable bit set.
79+
*
80+
* <p>We always treat files as executable since Bazel will `chmod 555` on the output files of an
81+
* action within ActionMetadataHandler#getMetadata after action execution if no metadata was
82+
* injected. We can't use real executable bit of the file until this behaviour is changed. See
83+
* https://github.com/bazelbuild/bazel/issues/13262 for more details.
84+
*/
85+
static FileNode createExecutable(String pathSegment, Path path, Digest digest) {
86+
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true);
87+
}
88+
89+
static FileNode createExecutable(String pathSegment, ByteString data, Digest digest) {
90+
return new FileNode(pathSegment, data, digest, /* isExecutable= */ true);
91+
}
92+
93+
private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
7894
super(pathSegment);
7995
this.path = Preconditions.checkNotNull(path, "path");
8096
this.data = null;
8197
this.digest = Preconditions.checkNotNull(digest, "digest");
8298
this.isExecutable = isExecutable;
8399
}
84100

85-
FileNode(String pathSegment, ByteString data, Digest digest) {
101+
private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable) {
86102
super(pathSegment);
87103
this.path = null;
88104
this.data = Preconditions.checkNotNull(data, "data");
89105
;
90106
this.digest = Preconditions.checkNotNull(digest, "digest");
91-
this.isExecutable = false;
107+
this.isExecutable = isExecutable;
92108
}
93109

94110
Digest getDigest() {

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

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.google.devtools.build.lib.actions.FileArtifactValue;
2121
import com.google.devtools.build.lib.actions.MetadataProvider;
2222
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
23-
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
2423
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
2524
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
2625
import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -102,7 +101,7 @@ private static int buildFromPaths(
102101
throw new IOException(String.format("Input '%s' is not a file.", input));
103102
}
104103
Digest d = digestUtil.compute(input);
105-
currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable()));
104+
currDir.addChild(FileNode.createExecutable(path.getBaseName(), input, d));
106105
return 1;
107106
});
108107
}
@@ -128,7 +127,8 @@ private static int buildFromActionInputs(
128127
if (input instanceof VirtualActionInput) {
129128
VirtualActionInput virtualActionInput = (VirtualActionInput) input;
130129
Digest d = digestUtil.compute(virtualActionInput);
131-
currDir.addChild(new FileNode(path.getBaseName(), virtualActionInput.getBytes(), d));
130+
currDir.addChild(
131+
FileNode.createExecutable(path.getBaseName(), virtualActionInput.getBytes(), d));
132132
return 1;
133133
}
134134

@@ -141,15 +141,7 @@ private static int buildFromActionInputs(
141141
case REGULAR_FILE:
142142
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
143143
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
144-
145-
boolean isExecutable;
146-
if (metadata instanceof RemoteActionFileArtifactValue) {
147-
isExecutable = ((RemoteActionFileArtifactValue) metadata).isExecutable();
148-
} else {
149-
isExecutable = inputPath.isExecutable();
150-
}
151-
152-
currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable));
144+
currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d));
153145
return 1;
154146

155147
case DIRECTORY:

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ public void virtualActionInputShouldWork() throws Exception {
7070
assertThat(directoriesAtDepth(1, tree)).isEmpty();
7171

7272
FileNode expectedFooNode =
73-
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
73+
FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
7474
FileNode expectedBarNode =
75-
new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
75+
FileNode.createExecutable("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
7676
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
7777
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
7878
}
@@ -117,19 +117,13 @@ public void directoryInputShouldBeExpanded() throws Exception {
117117
assertThat(directoriesAtDepth(3, tree)).isEmpty();
118118

119119
FileNode expectedFooNode =
120-
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
120+
FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
121121
FileNode expectedBarNode =
122-
new FileNode(
123-
"bar.cc",
124-
execRoot.getRelative(bar.getExecPath()),
125-
digestUtil.computeAsUtf8("bar"),
126-
false);
122+
FileNode.createExecutable(
123+
"bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"));
127124
FileNode expectedBuzzNode =
128-
new FileNode(
129-
"buzz.cc",
130-
execRoot.getRelative(buzz.getExecPath()),
131-
digestUtil.computeAsUtf8("buzz"),
132-
false);
125+
FileNode.createExecutable(
126+
"buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"));
133127
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
134128
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);
135129
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode);

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,12 @@ public void buildingATreeOfFilesShouldWork() throws Exception {
7474
assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz");
7575
assertThat(directoriesAtDepth(2, tree)).isEmpty();
7676

77-
FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false);
78-
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false);
77+
FileNode expectedFooNode =
78+
FileNode.createExecutable("foo.cc", foo, digestUtil.computeAsUtf8("foo"));
79+
FileNode expectedBarNode =
80+
FileNode.createExecutable("bar.cc", bar, digestUtil.computeAsUtf8("bar"));
7981
FileNode expectedBuzzNode =
80-
new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false);
82+
FileNode.createExecutable("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"));
8183
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
8284
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
8385
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode);

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,13 @@ public void buildMerkleTree() throws IOException {
8787

8888
Directory fizzDir =
8989
Directory.newBuilder()
90-
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), false))
91-
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), false))
90+
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), true))
91+
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), true))
9292
.build();
9393
Directory srcsDir =
9494
Directory.newBuilder()
95-
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), false))
96-
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), false))
95+
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), true))
96+
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), true))
9797
.addDirectories(
9898
DirectoryNode.newBuilder().setName("fizz").setDigest(digestUtil.compute(fizzDir)))
9999
.build();

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

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2194,26 +2194,70 @@ EOF
21942194
@local_foo//:all
21952195
}
21962196

2197-
function test_remote_input_files_executable_bit() {
2198-
# Test that input files uploaded to remote executor have the same executable bit with local files. #12818
2197+
function test_remote_cache_intermediate_outputs() {
2198+
# test that remote cache is hit when intermediate output is not executable
21992199
touch WORKSPACE
22002200
cat > BUILD <<'EOF'
2201+
genrule(
2202+
name = "dep",
2203+
srcs = [],
2204+
outs = ["dep"],
2205+
cmd = "echo 'dep' > $@",
2206+
)
2207+
22012208
genrule(
22022209
name = "test",
2203-
srcs = ["foo.txt", "bar.sh"],
2204-
outs = ["out.txt"],
2205-
cmd = "ls -l $(SRCS); touch $@",
2210+
srcs = [":dep"],
2211+
outs = ["out"],
2212+
cmd = "cat $(SRCS) > $@",
22062213
)
22072214
EOF
2208-
touch foo.txt bar.sh
2209-
chmod a+x bar.sh
22102215

22112216
bazel build \
2212-
--remote_executor=grpc://localhost:${worker_port} \
2217+
--remote_cache=grpc://localhost:${worker_port} \
2218+
//:test >& $TEST_log || fail "Failed to build //:test"
2219+
2220+
bazel clean
2221+
2222+
bazel build \
2223+
--remote_cache=grpc://localhost:${worker_port} \
2224+
//:test >& $TEST_log || fail "Failed to build //:test"
2225+
2226+
expect_log "2 remote cache hit"
2227+
}
2228+
2229+
function test_remote_cache_intermediate_outputs_toplevel() {
2230+
# test that remote cache is hit when intermediate output is not executable in remote download toplevel mode
2231+
touch WORKSPACE
2232+
cat > BUILD <<'EOF'
2233+
genrule(
2234+
name = "dep",
2235+
srcs = [],
2236+
outs = ["dep"],
2237+
cmd = "echo 'dep' > $@",
2238+
)
2239+
2240+
genrule(
2241+
name = "test",
2242+
srcs = [":dep"],
2243+
outs = ["out"],
2244+
cmd = "cat $(SRCS) > $@",
2245+
)
2246+
EOF
2247+
2248+
bazel build \
2249+
--remote_cache=grpc://localhost:${worker_port} \
2250+
--remote_download_toplevel \
2251+
//:test >& $TEST_log || fail "Failed to build //:test"
2252+
2253+
bazel clean
2254+
2255+
bazel build \
2256+
--remote_cache=grpc://localhost:${worker_port} \
2257+
--remote_download_toplevel \
22132258
//:test >& $TEST_log || fail "Failed to build //:test"
22142259

2215-
expect_log "-rwxr--r-- .* bar.sh"
2216-
expect_log "-rw-r--r-- .* foo.txt"
2260+
expect_log "2 remote cache hit"
22172261
}
22182262

22192263
function test_exclusive_tag() {

0 commit comments

Comments
 (0)