Skip to content

Commit

Permalink
[7.1.0] Set the executable bit on files in output directories uploade…
Browse files Browse the repository at this point in the history
…d to a disk or remote cache. (#21376)

Bazel doesn't preserve executable bits for outputs; all files in the
output tree are chmodded to either 0555 or 0755 after action execution,
depending on --experimental_writable_outputs.

With respect to the disk/remote cache protocol, Bazel always marks
uploaded inputs as executable and always ignores the executable bit on
downloaded outputs. For uploaded outputs, the behavior currently differs
between directories and non-directories; this CL makes the behavior
consistent.

This makes it more likely that the input Merkle tree to a remote action
can hit a cache populated by a previous local action. (See unknown
commit where the behavior was changed for non-directory outputs, with
the same rationale.)

As a minor effect, it also avoids additional I/O to obtain the
permission bits from the filesystem, which adds up for very large tree
artifacts.

PiperOrigin-RevId: 607367059
Change-Id: Ib507a98f32c0a5c89b1ed0f1ec3777f1c6430e28
  • Loading branch information
tjgq authored Feb 16, 2024
1 parent 6a19229 commit ec8db91
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 36 deletions.
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
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
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
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

0 comments on commit ec8db91

Please sign in to comment.