Remote: Make `chmod 0555` behavior consistent
After action execution, permission of output files is changed to [`0555`](https://github.com/bazelbuild/bazel/issues/5588). This PR updates remote module in following ways to make the behavior consistent:
1. Ignores `isExecutable` field of downloaded outputs from remote cache since the permission will be set to `0555` after action execution.
2. Always set `isExecutable` to `true` instead of reading the real permission bits from file system when uploading local outputs.
3. Do `chmod 0555` instead of `chmod 0755` when fetching inputs files for local actions which are outputs of previous remote actions. This should improve cache hit rate for builds that use dynamic execution and build without bytes.
Caveat: actions that depend on permission bits of input files (e.g. zip actions) shouldn't be executed dynamically since we have no control of input file permissions when running remotely. They should be always executed either locally or remotely.
b/198297058
Closes #13980.
PiperOrigin-RevId: 397257861
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
index 84cb444..efb85c4 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
@@ -173,9 +173,12 @@
private void finalizeDownload(Path path) {
try {
- path.chmod(0755);
+ // The permission of output file is changed to 0555 after action execution. We manually change
+ // the permission here
+ // for the downloaded file to keep this behaviour consistent.
+ path.chmod(0555);
} catch (IOException e) {
- logger.atWarning().withCause(e).log("Failed to chmod 755 on %s", path);
+ logger.atWarning().withCause(e).log("Failed to chmod 555 on %s", path);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index ad12441..1a4b123 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -648,10 +648,11 @@
*/
Collections.sort(finishedDownloads, Comparator.comparing(f -> toTmpDownloadPath(f.path())));
- // Move the output files from their temporary name to the actual output file name.
+ // Move the output files from their temporary name to the actual output file name. Executable
+ // bit
+ // is ignored since the file permission will be changed to 0555 after execution.
for (FileMetadata outputFile : finishedDownloads) {
FileSystemUtils.moveFile(toTmpDownloadPath(outputFile.path()), outputFile.path());
- outputFile.path().setExecutable(outputFile.isExecutable());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
index 28b622a..d2e7ba7 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
@@ -244,7 +244,8 @@
.addOutputFilesBuilder()
.setPath(remotePathResolver.localPathToOutputPath(file))
.setDigest(digest)
- .setIsExecutable(file.isExecutable());
+ // The permission of output file is changed to 0555 after action execution
+ .setIsExecutable(true);
digestToFile.put(digest, file);
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
index 06a35ad..1ec656f 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
@@ -453,7 +453,13 @@
ActionResult result = uploadDirectory(remoteCache, ImmutableList.<Path>of(fooFile, barDir));
ActionResult.Builder expectedResult = ActionResult.newBuilder();
- expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+ // output files will have permission 0555 after action execution regardless the current
+ // permission
+ expectedResult
+ .addOutputFilesBuilder()
+ .setPath("a/foo")
+ .setDigest(fooDigest)
+ .setIsExecutable(true);
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
assertThat(result).isEqualTo(expectedResult.build());
}
@@ -719,7 +725,13 @@
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.setStdoutDigest(stdoutDigest);
expectedResult.setStderrDigest(stderrDigest);
- expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+ // output files will have permission 0555 after action execution regardless the current
+ // permission
+ expectedResult
+ .addOutputFilesBuilder()
+ .setPath("a/foo")
+ .setDigest(fooDigest)
+ .setIsExecutable(true);
expectedResult
.addOutputFilesBuilder()
.setPath("bar")
@@ -778,7 +790,13 @@
command,
ImmutableList.of(fooFile, barFile));
ActionResult.Builder expectedResult = ActionResult.newBuilder();
- expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+ // output files will have permission 0555 after action execution regardless the current
+ // permission
+ expectedResult
+ .addOutputFilesBuilder()
+ .setPath("a/foo")
+ .setDigest(fooDigest)
+ .setIsExecutable(true);
expectedResult
.addOutputFilesBuilder()
.setPath("bar")
@@ -828,9 +846,11 @@
}
});
ActionResult.Builder rb = ActionResult.newBuilder();
- rb.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+ // output files will have permission 0555 after action execution regardless the current
+ // permission
+ rb.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest).setIsExecutable(true);
rb.addOutputFilesBuilder().setPath("bar").setDigest(barDigest).setIsExecutable(true);
- rb.addOutputFilesBuilder().setPath("baz").setDigest(bazDigest);
+ rb.addOutputFilesBuilder().setPath("baz").setDigest(bazDigest).setIsExecutable(true);
ActionResult result = rb.build();
serviceRegistry.addService(
new ActionCacheImplBase() {
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java
index 60e273a..1dc2494 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java
@@ -214,7 +214,7 @@
.isEqualTo("hello world");
assertThat(a1.getPath().isExecutable()).isTrue();
assertThat(a1.getPath().isReadable()).isTrue();
- assertThat(a1.getPath().isWritable()).isTrue();
+ assertThat(a1.getPath().isWritable()).isFalse();
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index 478752d..89eb7e7 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -163,8 +163,10 @@
}
@Test
- public void downloadOutputs_outputFiles_maintainsExecutableBit() throws Exception {
- // Test that downloading output files maintains executable bit works.
+ public void downloadOutputs_outputFiles_executableBitIgnored() throws Exception {
+ // Test that executable bit of downloaded output files are ignored since it will be chmod 555
+ // after action
+ // execution.
// arrange
Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
@@ -190,7 +192,7 @@
assertThat(digestUtil.compute(execRoot.getRelative("outputs/foo"))).isEqualTo(fooDigest);
assertThat(digestUtil.compute(execRoot.getRelative("outputs/bar"))).isEqualTo(barDigest);
assertThat(execRoot.getRelative("outputs/foo").isExecutable()).isFalse();
- assertThat(execRoot.getRelative("outputs/bar").isExecutable()).isTrue();
+ assertThat(execRoot.getRelative("outputs/bar").isExecutable()).isFalse();
assertThat(context.isLockOutputFilesCalled()).isTrue();
}
@@ -255,7 +257,7 @@
// assert
assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/foo"))).isEqualTo(fooDigest);
assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/bar/qux"))).isEqualTo(quxDigest);
- assertThat(execRoot.getRelative("outputs/a/bar/qux").isExecutable()).isTrue();
+ assertThat(execRoot.getRelative("outputs/a/bar/qux").isExecutable()).isFalse();
assertThat(context.isLockOutputFilesCalled()).isTrue();
}
@@ -1141,7 +1143,11 @@
// assert
ActionResult.Builder expectedResult = ActionResult.newBuilder();
- expectedResult.addOutputFilesBuilder().setPath("outputs/a/foo").setDigest(fooDigest);
+ expectedResult
+ .addOutputFilesBuilder()
+ .setPath("outputs/a/foo")
+ .setDigest(fooDigest)
+ .setIsExecutable(true);
expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest);
assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build());
diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
index 6b91370..6afd2b8 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
@@ -77,7 +77,7 @@
assertThat(um.getDigestToFile()).containsExactly(digest, link);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
- expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest);
+ expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
assertThat(result.build()).isEqualTo(expectedResult.build());
}
@@ -135,7 +135,7 @@
assertThat(um.getDigestToFile()).containsExactly(digest, link);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
- expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest);
+ expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
assertThat(result.build()).isEqualTo(expectedResult.build());
}
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 93c2864..5fa081f 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3029,6 +3029,72 @@
expect_log "ERROR: Failed to query remote execution capabilities: Failed to init TLS infrastructure using '/nope' as root certificate: File does not contain valid certificates: /nope"
}
-run_suite "Remote execution and remote cache tests"
+function test_output_file_permission() {
+ # Test that permission of output files are always 0555
+ mkdir -p a
+ cat > a/BUILD <<EOF
+genrule(
+ name = "foo",
+ srcs = [],
+ outs = ["foo"],
+ cmd = "echo 'foo' > \$@",
+)
+
+genrule(
+ name = "bar",
+ srcs = [":foo"],
+ outs = ["bar"],
+ cmd = "ls -lL \$(SRCS) > \$@",
+ tags = ["no-remote"],
+)
+EOF
+
+ # no remote execution
+ bazel build \
+ //a:bar >& $TEST_log || fail "Failed to build"
+
+ ls -l bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ ls -l bazel-bin/a/foo >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ cat bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ bazel clean >& $TEST_log || fail "Failed to clean"
+
+ # normal remote execution
+ bazel build \
+ --remote_executor=grpc://localhost:${worker_port} \
+ //a:bar >& $TEST_log || fail "Failed to build"
+
+ ls -l bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ ls -l bazel-bin/a/foo >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ cat bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ bazel clean >& $TEST_log || fail "Failed to clean"
+
+ # build without bytes
+ bazel build \
+ --remote_executor=grpc://localhost:${worker_port} \
+ --remote_download_minimal \
+ //a:bar >& $TEST_log || fail "Failed to build"
+
+ ls -l bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ ls -l bazel-bin/a/foo >& $TEST_log
+ expect_log "-r-xr-xr-x"
+
+ cat bazel-bin/a/bar >& $TEST_log
+ expect_log "-r-xr-xr-x"
}
+
+run_suite "Remote execution and remote cache tests"
