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);
}
