Skip to content

Commit 4247c20

Browse files
tjgqcopybara-github
tjgq
authored and
copybara-github
committed
Traverse symlinks to directories while collecting a TreeArtifactValue.
As explained in #20418, when a tree artifact contains a symlink to a directory, it is collected as a single TreeFileArtifact with DirectoryArtifactValue metadata. With this change, the symlink is followed and the directory expanded into its contents, which is more incrementally correct and removes a special case that tree artifact consumers would otherwise have to be aware of. This also addresses #21171, which is due to the metadata for the directory contents not being available when building without the bytes, causing the input Merkle tree builder to fail. (While I could have fixed this by falling back to reading the directory contents from the filesystem, I prefer to abide by the principle that input metadata should be collected before execution; source directories are the other case where this isn't true, which I also regard as a bug.) Fixes #20418. Fixes #21171. PiperOrigin-RevId: 608389141 Change-Id: I956f3f8a4b1bfd279091e179d1cba3cdd0e5019b
1 parent 9eb3b0a commit 4247c20

8 files changed

Lines changed: 350 additions & 75 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -289,26 +289,19 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
289289

290290
TreeArtifactValue.visitTree(
291291
treeDir,
292-
(parentRelativePath, type) -> {
293-
if (chmod && type != Dirent.Type.SYMLINK) {
292+
(parentRelativePath, type, traversedSymlink) -> {
293+
checkState(type == Dirent.Type.FILE || type == Dirent.Type.DIRECTORY);
294+
// Set the output permissions when the execution mode requires it, unless at least one
295+
// symlink was traversed on the way to this entry, as it might have led outside of the
296+
// root directory.
297+
if (chmod && !traversedSymlink) {
294298
setPathPermissions(treeDir.getRelative(parentRelativePath));
295299
}
296300
if (type == Dirent.Type.DIRECTORY) {
297301
return; // The final TreeArtifactValue does not contain child directories.
298302
}
299303
TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, parentRelativePath);
300-
FileArtifactValue metadata;
301-
try {
302-
metadata = constructFileArtifactValueFromFilesystem(child);
303-
} catch (FileNotFoundException e) {
304-
String errorMessage =
305-
String.format(
306-
"Failed to resolve relative path %s inside TreeArtifact %s. "
307-
+ "The associated file is either missing or is an invalid symlink.",
308-
parentRelativePath, treeDir);
309-
throw new IOException(errorMessage, e);
310-
}
311-
304+
FileArtifactValue metadata = constructFileArtifactValueFromFilesystem(child);
312305
// visitTree() uses multiple threads and putChild() is not thread-safe
313306
synchronized (tree) {
314307
if (metadata.isRemote()) {

‎src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,7 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
464464
try {
465465
TreeArtifactValue.visitTree(
466466
path,
467-
(child, type) -> {
467+
(child, type, traversedSymlink) -> {
468468
if (type != Dirent.Type.DIRECTORY) {
469469
currentChildren.add(child);
470470
}

‎src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java‎

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
4444
import com.google.devtools.build.lib.util.Fingerprint;
4545
import com.google.devtools.build.lib.vfs.Dirent;
46+
import com.google.devtools.build.lib.vfs.FileStatus;
4647
import com.google.devtools.build.lib.vfs.IORuntimeException;
4748
import com.google.devtools.build.lib.vfs.Path;
4849
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -487,20 +488,34 @@ public interface TreeArtifactVisitor {
487488
* Called for every directory entry encountered during tree traversal, in a nondeterministic
488489
* order.
489490
*
490-
* <p>Symlinks are not followed during traversal and are simply reported as {@link
491-
* Dirent.Type#SYMLINK} regardless of whether they point to a file, directory, or are dangling.
491+
* <p>Regular files and directories are reported as {@link Dirent.Type.FILE} or {@link
492+
* Dirent.Type.DIRECTORY}, respectively. Directories are traversed recursively.
492493
*
493-
* <p>{@code type} is guaranteed never to be {@link Dirent.Type#UNKNOWN}, since if this type is
494-
* encountered, {@link IOException} is immediately thrown without invoking the visitor.
494+
* <p>Symlinks that resolve to an existing file or directory are followed and reported as the
495+
* regular files or directories they point to, recursively for directories. Symlinks that fail
496+
* to resolve to an existing path cause an {@link IOException} to be immediately thrown without
497+
* invoking the visitor. Thus, the visitor is never called with a {@link Dirent.Type.SYMLINK}
498+
* type.
495499
*
496-
* <p>If the implementation throws {@link IOException}, traversal is immediately halted and the
500+
* <p>Special files or files whose type could not be determined, regardless of whether they are
501+
* encountered directly or indirectly through symlinks, cause an {@link IOException} to be
502+
* immediately thrown without invoking the visitor. Thus, the visitor is never called with a
503+
* {@link Dirent.Type.UNKNOWN} type.
504+
*
505+
* <p>The {@code parentRelativePath} argument is always set to the apparent path relative to the
506+
* tree directory root, without resolving any intervening symlinks. The {@code traversedSymlink}
507+
* argument is true if at least one symlink was traversed on the way to the entry being
508+
* reported.
509+
*
510+
* <p>If the visitor throws {@link IOException}, traversal is immediately halted and the
497511
* exception is propagated.
498512
*
499513
* <p>This method can be called from multiple threads in parallel during a single call of {@link
500514
* TreeArtifactVisitor#visitTree(Path, TreeArtifactVisitor)}.
501515
*/
502516
@ThreadSafe
503-
void visit(PathFragment parentRelativePath, Dirent.Type type) throws IOException;
517+
void visit(PathFragment parentRelativePath, Dirent.Type type, boolean traversedSymlink)
518+
throws IOException;
504519
}
505520

506521
/** An {@link AbstractQueueVisitor} that visits every file in the tree artifact. */
@@ -519,33 +534,62 @@ static class Visitor extends AbstractQueueVisitor {
519534
}
520535

521536
void run() throws IOException, InterruptedException {
522-
execute(() -> visit(PathFragment.EMPTY_FRAGMENT, Dirent.Type.DIRECTORY));
537+
execute(
538+
() ->
539+
visit(
540+
PathFragment.EMPTY_FRAGMENT,
541+
Dirent.Type.DIRECTORY,
542+
/* traversedSymlink= */ false));
523543
try {
524544
awaitQuiescence(true);
525545
} catch (IORuntimeException e) {
526546
throw e.getCauseIOException();
527547
}
528548
}
529549

530-
private void visit(PathFragment parentRelativePath, Dirent.Type type) {
550+
private void visit(
551+
PathFragment parentRelativePath, Dirent.Type type, boolean traversedSymlink) {
531552
try {
532553
Path path = parentDir.getRelative(parentRelativePath);
533554

534-
if (type == Dirent.Type.UNKNOWN) {
535-
throw new IOException("Could not determine type of file for " + path.getPathString());
536-
}
537-
538555
if (type == Dirent.Type.SYMLINK) {
539556
checkSymlink(parentRelativePath.getParentDirectory(), path);
557+
558+
traversedSymlink = true;
559+
560+
FileStatus statFollow = path.statIfFound(Symlinks.FOLLOW);
561+
562+
if (statFollow == null) {
563+
throw new IOException(
564+
String.format(
565+
"Child %s of tree artifact %s is a dangling symbolic link",
566+
parentRelativePath, parentDir));
567+
}
568+
569+
if (statFollow.isFile() && !statFollow.isSpecialFile()) {
570+
type = Dirent.Type.FILE;
571+
} else if (statFollow.isDirectory()) {
572+
type = Dirent.Type.DIRECTORY;
573+
} else {
574+
type = Dirent.Type.UNKNOWN;
575+
}
576+
}
577+
578+
if (type == Dirent.Type.UNKNOWN) {
579+
throw new IOException(
580+
String.format(
581+
"Child %s of tree artifact %s has an unsupported type",
582+
parentRelativePath, parentDir));
540583
}
541584

542-
visitor.visit(parentRelativePath, type);
585+
visitor.visit(parentRelativePath, type, traversedSymlink);
543586

544587
if (type == Dirent.Type.DIRECTORY) {
545588
for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) {
546589
PathFragment childPath = parentRelativePath.getChild(dirent.getName());
547590
Dirent.Type childType = dirent.getType();
548-
execute(() -> visit(childPath, childType));
591+
boolean finalTraversedSymlink = traversedSymlink;
592+
execute(() -> visit(childPath, childType, finalTraversedSymlink));
549593
}
550594
}
551595
} catch (IOException e) {

‎src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1655,7 +1655,7 @@ public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output)
16551655
if (treeDir.exists()) {
16561656
TreeArtifactValue.visitTree(
16571657
treeDir,
1658-
(parentRelativePath, type) -> {
1658+
(parentRelativePath, type, traversedSymlink) -> {
16591659
if (type == Dirent.Type.DIRECTORY) {
16601660
return;
16611661
}

‎src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -975,7 +975,7 @@ protected static TreeArtifactValue createTreeArtifactValue(Artifact tree) throws
975975
TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder((SpecialArtifact) tree);
976976
TreeArtifactValue.visitTree(
977977
tree.getPath(),
978-
(parentRelativePath, type) -> {
978+
(parentRelativePath, type, traversedSymlink) -> {
979979
if (type.equals(Dirent.Type.DIRECTORY)) {
980980
return;
981981
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -676,4 +676,34 @@ public void downloadTopLevel_genruleSymlinkToOutput() throws Exception {
676676
assertSymlink("foo-link", PathFragment.create("foo"));
677677
assertValidOutputFile("foo-link", "hello\n");
678678
}
679+
680+
@Test
681+
public void remoteAction_inputTreeWithSymlinks() throws Exception {
682+
setDownloadToplevel();
683+
write(
684+
"tree.bzl",
685+
"def _impl(ctx):",
686+
" d = ctx.actions.declare_directory(ctx.label.name)",
687+
" ctx.actions.run_shell(",
688+
" outputs = [d],",
689+
" command = 'mkdir $1/dir && touch $1/file $1/dir/file && ln -s file $1/filesym && ln"
690+
+ " -s dir $1/dirsym',",
691+
" arguments = [d.path],",
692+
" )",
693+
" return DefaultInfo(files = depset([d]))",
694+
"tree = rule(_impl)");
695+
write(
696+
"BUILD",
697+
"load(':tree.bzl', 'tree')",
698+
"tree(name = 'tree')",
699+
"genrule(name = 'gen', srcs = [':tree'], outs = ['out'], cmd = 'touch $@')");
700+
701+
// Populate cache
702+
buildTarget("//:gen");
703+
704+
// Delete output, replay from cache
705+
getOutputPath("tree").deleteTree();
706+
getOutputPath("out").delete();
707+
buildTarget("//:gen");
708+
}
679709
}

‎src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java‎

Lines changed: 91 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,61 @@ void run(ActionExecutionContext context) throws IOException {
404404
checkFilePermissions(out.getPath().getChild("three").getChild("four"));
405405
}
406406

407+
@Test
408+
public void doesNotSetPermissionsAfterTraversingSymlink() throws Exception {
409+
SpecialArtifact out = createTreeArtifact("output");
410+
411+
Path fileTarget = scratch.file("file");
412+
writeFile(fileTarget, "file");
413+
414+
Path dirTarget = scratch.dir("dir");
415+
Path dirFileTarget = dirTarget.getChild("file");
416+
writeFile(dirFileTarget, "dir/file");
417+
418+
Action action =
419+
new SimpleTestAction(out) {
420+
@Override
421+
void run(ActionExecutionContext context) throws IOException {
422+
out.getPath().getChild("file_link").createSymbolicLink(fileTarget.asFragment());
423+
out.getPath().getChild("dir_link").createSymbolicLink(dirTarget.asFragment());
424+
}
425+
};
426+
427+
registerAction(action);
428+
buildArtifact(out);
429+
430+
assertThat(fileTarget.isWritable()).isTrue();
431+
assertThat(dirTarget.isWritable()).isTrue();
432+
assertThat(dirFileTarget.isWritable()).isTrue();
433+
}
434+
435+
@Test
436+
public void symlinkLoopRejected() throws Exception {
437+
// Failure expected
438+
EventCollector eventCollector = new EventCollector(EventKind.ERROR);
439+
reporter.removeHandler(failFastHandler);
440+
reporter.addHandler(eventCollector);
441+
442+
SpecialArtifact out = createTreeArtifact("output");
443+
444+
Action action =
445+
new SimpleTestAction(out) {
446+
@Override
447+
void run(ActionExecutionContext context) throws IOException {
448+
writeFile(out.getPath().getRelative("dir/file"), "contents");
449+
out.getPath().getRelative("dir/sym").createSymbolicLink(PathFragment.create("../dir"));
450+
}
451+
};
452+
453+
registerAction(action);
454+
assertThrows(BuildFailedException.class, () -> buildArtifact(out));
455+
456+
ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
457+
assertThat(errors).hasSize(2);
458+
assertThat(errors.get(0).getMessage()).contains("Too many levels of symbolic links");
459+
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
460+
}
461+
407462
@Test
408463
public void validRelativeSymlinkAccepted() throws Exception {
409464
SpecialArtifact out = createTreeArtifact("output");
@@ -448,7 +503,9 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
448503

449504
List<Event> errors = ImmutableList.copyOf(eventCollector);
450505
assertThat(errors).hasSize(2);
451-
assertThat(errors.get(0).getMessage()).contains("Failed to resolve relative path links/link");
506+
assertThat(errors.get(0).getMessage())
507+
.contains(
508+
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
452509
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
453510
}
454511

@@ -477,7 +534,9 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
477534

478535
List<Event> errors = ImmutableList.copyOf(eventCollector);
479536
assertThat(errors).hasSize(2);
480-
assertThat(errors.get(0).getMessage()).contains("Failed to resolve relative path links/link");
537+
assertThat(errors.get(0).getMessage())
538+
.contains(
539+
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
481540
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
482541
}
483542

@@ -852,17 +911,18 @@ public void emptyInputAndOutputTreeArtifactInActionTemplate() throws Exception {
852911
assertThat(artifact2.getPath().getDirectoryEntries()).isEmpty();
853912
}
854913

855-
// This happens in the wild. See https://github.com/bazelbuild/bazel/issues/11813.
856914
@Test
857-
public void treeArtifactContainsSymlinkToDirectory() throws Exception {
915+
public void treeArtifactWithSymlinkToFile() throws Exception {
858916
SpecialArtifact treeArtifact = createTreeArtifact("tree");
859917
registerAction(
860-
new SimpleTestAction(/*output=*/ treeArtifact) {
918+
new SimpleTestAction(/* output= */ treeArtifact) {
861919
@Override
862920
void run(ActionExecutionContext context) throws IOException {
863-
PathFragment subdir = PathFragment.create("subdir");
864-
touchFile(treeArtifact.getPath().getRelative(subdir).getRelative("file"));
865-
treeArtifact.getPath().getRelative("link").createSymbolicLink(subdir);
921+
touchFile(treeArtifact.getPath().getRelative("subdir/file"));
922+
treeArtifact
923+
.getPath()
924+
.getRelative("link")
925+
.createSymbolicLink(PathFragment.create("subdir/file"));
866926
}
867927
});
868928

@@ -874,6 +934,29 @@ void run(ActionExecutionContext context) throws IOException {
874934
TreeFileArtifact.createTreeOutput(treeArtifact, "link"));
875935
}
876936

937+
@Test
938+
public void treeArtifactWithSymlinkToDirectory() throws Exception {
939+
SpecialArtifact treeArtifact = createTreeArtifact("tree");
940+
registerAction(
941+
new SimpleTestAction(/* output= */ treeArtifact) {
942+
@Override
943+
void run(ActionExecutionContext context) throws IOException {
944+
touchFile(treeArtifact.getPath().getRelative("subdir/file"));
945+
treeArtifact
946+
.getPath()
947+
.getRelative("link")
948+
.createSymbolicLink(PathFragment.create("subdir"));
949+
}
950+
});
951+
952+
TreeArtifactValue tree = buildArtifact(treeArtifact);
953+
954+
assertThat(tree.getChildren())
955+
.containsExactly(
956+
TreeFileArtifact.createTreeOutput(treeArtifact, "subdir/file"),
957+
TreeFileArtifact.createTreeOutput(treeArtifact, "link/file"));
958+
}
959+
877960
private abstract static class SimpleTestAction extends TestAction {
878961
private final Button button;
879962

0 commit comments

Comments
 (0)