Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Set the executable bit on files in output directories uploaded to a disk or remote cache. #21376

Merged
merged 1 commit into from
Feb 16, 2024
Merged

[7.1.0] Set the executable bit on files in output directories uploaded to a disk or remote cache. #21376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ private void setStdoutStderr(FileOutErr outErr) throws IOException {
* non-symlink where a symlink was expected). Outputs are always uploaded according to the
* filesystem state, possibly after applying the transformation implied by {@link followSymlinks}.
* A type mismatch may later cause execution to fail, but that's an action-level concern.
*
* <p>All files are uploaded with the executable bit set, in accordance with input Merkle trees.
* This does not affect correctness since we always set the output permissions to 0555 or 0755
* after execution, both for cache hits and misses.
*/
@VisibleForTesting
void addFiles(Collection<Path> files) throws ExecException, IOException {
Expand Down Expand Up @@ -332,7 +336,6 @@ private void addFile(Digest digest, Path file) throws IOException {
.addOutputFilesBuilder()
.setPath(remotePathResolver.localPathToOutputPath(file))
.setDigest(digest)
// The permission of output file is changed to 0555 after action execution
.setIsExecutable(true);

digestToFile.put(digest, file);
Expand Down Expand Up @@ -395,7 +398,7 @@ private ByteString computeDirectory(Path path, LinkedHashSet<ByteString> directo
Path child = path.getRelative(name);
if (dirent.getType() == Dirent.Type.FILE) {
Digest digest = digestUtil.compute(child);
b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(true);
digestToFile.put(digest, child);
continue;
}
Expand All @@ -421,7 +424,7 @@ private ByteString computeDirectory(Path path, LinkedHashSet<ByteString> directo
if (statFollow.isFile() && !statFollow.isSpecialFile()) {
// Symlink to file uploaded as a file.
Digest digest = digestUtil.compute(child);
b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(true);
digestToFile.put(digest, child);
continue;
}
Expand Down
11 changes: 8 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,12 @@ public void testUploadDirectoryNested() throws Exception {
fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/qux"), "abc");
final Directory testDirMessage =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("wobble").setDigest(wobbleDigest).build())
.addFiles(
FileNode.newBuilder()
.setName("wobble")
.setDigest(wobbleDigest)
.setIsExecutable(true)
.build())
.build();
final Digest testDigest = DIGEST_UTIL.compute(testDirMessage);
final Tree barTree =
Expand All @@ -649,9 +654,9 @@ public void testUploadDirectoryNested() throws Exception {
Directory.newBuilder()
.addFiles(
FileNode.newBuilder()
.setIsExecutable(true)
.setName("qux")
.setDigest(quxDigest))
.setDigest(quxDigest)
.setIsExecutable(true))
.addDirectories(
DirectoryNode.newBuilder().setName("test").setDigest(testDigest)))
.addChildren(testDirMessage)
Expand Down
84 changes: 64 additions & 20 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,11 @@ public void downloadOutputs_executableBitIgnored() throws Exception {
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("bar").setDigest(barDigest)))
.addFiles(
FileNode.newBuilder()
.setName("bar")
.setDigest(barDigest)
.setIsExecutable(true)))
.build();
Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
ActionResult.Builder builder = ActionResult.newBuilder();
Expand Down Expand Up @@ -401,8 +405,16 @@ public void downloadOutputs_outputDirectories() throws Exception {
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(fooDigest))
.addFiles(FileNode.newBuilder().setName("subdir/bar").setDigest(barDigest)))
.addFiles(
FileNode.newBuilder()
.setName("foo")
.setDigest(fooDigest)
.setIsExecutable(true))
.addFiles(
FileNode.newBuilder()
.setName("subdir/bar")
.setDigest(barDigest)
.setIsExecutable(true)))
.build();
Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
ActionResult.Builder builder = ActionResult.newBuilder();
Expand Down Expand Up @@ -469,15 +481,20 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception {
Digest quxDigest = cache.addContents(remoteActionExecutionContext, "qux-contents");
Directory wobbleDirMessage =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("qux").setDigest(quxDigest))
.addFiles(
FileNode.newBuilder().setName("qux").setDigest(quxDigest).setIsExecutable(true))
.build();
Digest wobbleDirDigest =
cache.addContents(remoteActionExecutionContext, wobbleDirMessage.toByteArray());
Tree barTreeMessage =
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("qux").setDigest(quxDigest))
.addFiles(
FileNode.newBuilder()
.setName("qux")
.setDigest(quxDigest)
.setIsExecutable(true))
.addDirectories(
DirectoryNode.newBuilder().setName("wobble").setDigest(wobbleDirDigest)))
.addChildren(wobbleDirMessage)
Expand Down Expand Up @@ -518,8 +535,16 @@ public void downloadOutputs_outputDirectoriesWithNestedFile_works() throws Excep
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(fooDigest))
.addFiles(FileNode.newBuilder().setName("bar").setDigest(barDigest)))
.addFiles(
FileNode.newBuilder()
.setName("foo")
.setDigest(fooDigest)
.setIsExecutable(true))
.addFiles(
FileNode.newBuilder()
.setName("bar")
.setDigest(barDigest)
.setIsExecutable(true)))
.build();
Digest subdirTreeDigest =
cache.addContents(remoteActionExecutionContext, subdirTreeMessage.toByteArray());
Expand Down Expand Up @@ -559,7 +584,8 @@ public void downloadOutputs_outputDirectoriesWithSameHash_works() throws Excepti

// arrange
Digest fileDigest = cache.addContents(remoteActionExecutionContext, "file");
FileNode file = FileNode.newBuilder().setName("file").setDigest(fileDigest).build();
FileNode file =
FileNode.newBuilder().setName("file").setDigest(fileDigest).setIsExecutable(true).build();
Directory fooDir = Directory.newBuilder().addFiles(file).build();
Digest fooDigest = cache.addContents(remoteActionExecutionContext, fooDir.toByteArray());
DirectoryNode fooDirNode =
Expand Down Expand Up @@ -815,7 +841,11 @@ public void downloadOutputs_onActionFailure_downloadEverything() throws Exceptio
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("bar").setDigest(barDigest)))
.addFiles(
FileNode.newBuilder()
.setName("bar")
.setDigest(barDigest)
.setIsExecutable(true)))
.build();
Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
ActionResult.Builder builder = ActionResult.newBuilder();
Expand Down Expand Up @@ -850,7 +880,10 @@ public void downloadOutputs_onDownloadFailure_maintainDirectories() throws Excep
.setRoot(
Directory.newBuilder()
.addFiles(
FileNode.newBuilder().setName("outputfile").setDigest(treeFileDigest)))
FileNode.newBuilder()
.setName("outputfile")
.setDigest(treeFileDigest)
.setIsExecutable(true)))
.build();
Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
Digest otherFileDigest =
Expand Down Expand Up @@ -1201,8 +1234,10 @@ public void downloadOutputs_outputDirectories_partialDownload() throws Exception
// dir/a/file2
Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
Digest d2 = cache.addContents(remoteActionExecutionContext, "content2");
FileNode file1 = FileNode.newBuilder().setName("file1").setDigest(d1).build();
FileNode file2 = FileNode.newBuilder().setName("file2").setDigest(d2).build();
FileNode file1 =
FileNode.newBuilder().setName("file1").setDigest(d1).setIsExecutable(true).build();
FileNode file2 =
FileNode.newBuilder().setName("file2").setDigest(d2).setIsExecutable(true).build();
Directory a = Directory.newBuilder().addFiles(file2).build();
Digest da = cache.addContents(remoteActionExecutionContext, a);
Directory root =
Expand Down Expand Up @@ -1251,8 +1286,10 @@ public void downloadOutputs_outputDirectories_noDownload() throws Exception {
// dir/a/file2
Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
Digest d2 = cache.addContents(remoteActionExecutionContext, "content2");
FileNode file1 = FileNode.newBuilder().setName("file1").setDigest(d1).build();
FileNode file2 = FileNode.newBuilder().setName("file2").setDigest(d2).build();
FileNode file1 =
FileNode.newBuilder().setName("file1").setDigest(d1).setIsExecutable(true).build();
FileNode file2 =
FileNode.newBuilder().setName("file2").setDigest(d2).setIsExecutable(true).build();
Directory a = Directory.newBuilder().addFiles(file2).build();
Digest da = cache.addContents(remoteActionExecutionContext, a);
Directory root =
Expand Down Expand Up @@ -1303,8 +1340,10 @@ public void downloadOutputs_outputDirectories_doNotDownload_failProperly() throw
// dir/a/file2
Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
Digest d2 = cache.addContents(remoteActionExecutionContext, "content2");
FileNode file1 = FileNode.newBuilder().setName("file1").setDigest(d1).build();
FileNode file2 = FileNode.newBuilder().setName("file2").setDigest(d2).build();
FileNode file1 =
FileNode.newBuilder().setName("file1").setDigest(d1).setIsExecutable(true).build();
FileNode file2 =
FileNode.newBuilder().setName("file2").setDigest(d2).setIsExecutable(true).build();
Directory a = Directory.newBuilder().addFiles(file2).build();
Digest da = cache.addContents(remoteActionExecutionContext, a);
Directory root =
Expand Down Expand Up @@ -1577,9 +1616,9 @@ public void uploadOutputs_uploadDirectory_works() throws Exception {
Directory.newBuilder()
.addFiles(
FileNode.newBuilder()
.setIsExecutable(true)
.setName("qux")
.setDigest(quxDigest)
.setIsExecutable(true)
.build())
.build())
.build());
Expand Down Expand Up @@ -1680,7 +1719,12 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception {
fakeFileCache.createScratchInput(ActionInputHelper.fromPath("outputs/bar/qux"), "abc");
final Directory testDirMessage =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("wobble").setDigest(wobbleDigest).build())
.addFiles(
FileNode.newBuilder()
.setName("wobble")
.setDigest(wobbleDigest)
.setIsExecutable(true)
.build())
.build();
final Digest testDigest = digestUtil.compute(testDirMessage);
final Tree barTree =
Expand All @@ -1689,9 +1733,9 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception {
Directory.newBuilder()
.addFiles(
FileNode.newBuilder()
.setIsExecutable(true)
.setName("qux")
.setDigest(quxDigest))
.setDigest(quxDigest)
.setIsExecutable(true))
.addDirectories(
DirectoryNode.newBuilder().setName("test").setDigest(testDigest)))
.addChildren(testDirMessage)
Expand Down
47 changes: 37 additions & 10 deletions src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,11 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkAsDirectory() th
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("foo")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -311,7 +315,11 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkAsDirectory()
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("foo")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -375,7 +383,11 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkAsDirectory() th
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("foo")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -631,7 +643,11 @@ public void actionResult_followSymlinks_absoluteFileSymlinkInDirectoryAsFile() t
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("link").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("link")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -671,7 +687,7 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkInDirectoryAsDir

Directory barDir =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest).setIsExecutable(true))
.build();
Digest barDigest = digestUtil.compute(barDir);
Tree tree =
Expand Down Expand Up @@ -720,7 +736,11 @@ public void actionResult_noFollowSymlinks_absoluteFileSymlinkInDirectoryAsFile()
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("link").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("link")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -760,7 +780,7 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkInDirectoryAsD

Directory barDir =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest).setIsExecutable(true))
.build();
Digest barDigest = digestUtil.compute(barDir);
Tree tree =
Expand Down Expand Up @@ -808,7 +828,11 @@ public void actionResult_followSymlinks_relativeFileSymlinkInDirectoryAsFile() t
Tree.newBuilder()
.setRoot(
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("link").setDigest(digest)))
.addFiles(
FileNode.newBuilder()
.setName("link")
.setDigest(digest)
.setIsExecutable(true)))
.build();
Digest treeDigest = digestUtil.compute(tree);

Expand Down Expand Up @@ -848,7 +872,7 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkInDirectoryAsDir

Directory barDir =
Directory.newBuilder()
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))
.addFiles(FileNode.newBuilder().setName("foo").setDigest(digest).setIsExecutable(true))
.build();
Digest barDigest = digestUtil.compute(barDir);
Tree tree =
Expand Down Expand Up @@ -1278,7 +1302,10 @@ public void actionResult_topologicallySortedAndDeduplicatedTree() throws Excepti
Directory root =
Directory.newBuilder()
.addFiles(
FileNode.newBuilder().setName("file").setDigest(digestUtil.compute(fileContents)))
FileNode.newBuilder()
.setName("file")
.setDigest(digestUtil.compute(fileContents))
.setIsExecutable(true))
.build();
for (int depth = 0; depth < 3; depth++) {
Directory.Builder b = Directory.newBuilder();
Expand Down
Loading