Skip to content

Commit 33a2ce6

Browse files
bazel-iotjgq
andauthored
[7.1.2] Implement RemoteActionFileSystem#statIfFound correctly when the path cannot be canonicalized (#21889)
This improves the error message for a tree artifact containing a dangling symlink, which regressed in 4247c20 (see #15454 (comment)). PiperOrigin-RevId: 617870632 Change-Id: I6847084a52b1e4bb7d8a9384ad6cd5d015dddf1b Commit b78d73f Co-authored-by: Googler <[email protected]>
1 parent e9bc5ec commit 33a2ce6

File tree

7 files changed

+69
-34
lines changed

7 files changed

+69
-34
lines changed

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -630,13 +630,17 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) {
630630
private FileStatus statInternal(PathFragment path, FollowMode followMode, StatSources statSources)
631631
throws IOException {
632632
// Canonicalize the path.
633-
if (followMode == FollowMode.FOLLOW_ALL) {
634-
path = resolveSymbolicLinks(path).asFragment();
635-
} else if (followMode == FollowMode.FOLLOW_PARENT) {
636-
PathFragment parent = path.getParentDirectory();
637-
if (parent != null) {
638-
path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
633+
try {
634+
if (followMode == FollowMode.FOLLOW_ALL) {
635+
path = resolveSymbolicLinks(path).asFragment();
636+
} else if (followMode == FollowMode.FOLLOW_PARENT) {
637+
PathFragment parent = path.getParentDirectory();
638+
if (parent != null) {
639+
path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
640+
}
639641
}
642+
} catch (FileNotFoundException e) {
643+
return null;
640644
}
641645

642646
// Since the path has been canonicalized, the operations below never need to follow symlinks.

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1488,11 +1488,12 @@ private static void reportOutputTreeArtifactErrors(
14881488
Action action, Artifact output, Reporter reporter, IOException e) {
14891489
String errorMessage;
14901490
if (e instanceof FileNotFoundException) {
1491-
errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint());
1491+
errorMessage = String.format("output tree artifact %s was not created", output.prettyPrint());
14921492
} else {
14931493
errorMessage =
14941494
String.format(
1495-
"Error while validating output TreeArtifact %s : %s", output, e.getMessage());
1495+
"error while validating output tree artifact %s: %s",
1496+
output.prettyPrint(), e.getMessage());
14961497
}
14971498

14981499
reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -558,9 +558,7 @@ private void visit(
558558

559559
if (statFollow == null) {
560560
throw new IOException(
561-
String.format(
562-
"Child %s of tree artifact %s is a dangling symbolic link",
563-
parentRelativePath, parentDir));
561+
String.format("child %s is a dangling symbolic link", parentRelativePath));
564562
}
565563

566564
if (statFollow.isFile() && !statFollow.isSpecialFile()) {
@@ -574,9 +572,7 @@ private void visit(
574572

575573
if (type == Dirent.Type.UNKNOWN) {
576574
throw new IOException(
577-
String.format(
578-
"Child %s of tree artifact %s has an unsupported type",
579-
parentRelativePath, parentDir));
575+
String.format("child %s has an unsupported type", parentRelativePath));
580576
}
581577

582578
visitor.visit(parentRelativePath, type, traversedSymlink);

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,11 +451,43 @@ public void statAndExists_followSymlinks(
451451
@Test
452452
public void statAndExists_notFound() throws Exception {
453453
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
454-
Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
455-
PathFragment path = artifact.getPath().asFragment();
454+
PathFragment path = getOutputPath("does_not_exist");
455+
456+
assertThat(actionFs.exists(path)).isFalse();
457+
458+
assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
459+
460+
assertThrows(
461+
FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
462+
}
463+
464+
@Test
465+
public void statAndExists_isNotDirectory() throws Exception {
466+
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
467+
PathFragment nonDirPath = getOutputPath("non_dir");
468+
PathFragment path = nonDirPath.getChild("file");
469+
470+
writeLocalFile(actionFs, nonDirPath, "content");
456471

457472
assertThat(actionFs.exists(path)).isFalse();
458473

474+
assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
475+
476+
assertThrows(
477+
FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
478+
}
479+
480+
@Test
481+
public void statAndExists_danglingSymlink_notFound() throws Exception {
482+
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
483+
PathFragment path = getOutputPath("sym");
484+
485+
actionFs.getPath(path).createSymbolicLink(PathFragment.create("/does_not_exist"));
486+
487+
assertThat(actionFs.exists(path)).isFalse();
488+
489+
assertThat(actionFs.statIfFound(path, /* followSymlinks= */ true)).isNull();
490+
459491
assertThrows(
460492
FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
461493
}

src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,20 @@ public void createsTreeArtifactValueFromFilesystem() throws Exception {
259259
assertThat(chmodCalls).isEmpty();
260260
}
261261

262+
@Test
263+
public void withDanglingSymlinkInTreeArtifactFailsWithException() throws Exception {
264+
SpecialArtifact treeArtifact =
265+
ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "foo/bar");
266+
TreeFileArtifact child = TreeFileArtifact.createTreeOutput(treeArtifact, "child");
267+
treeArtifact.getPath().createDirectoryAndParents();
268+
child.getPath().createSymbolicLink(PathFragment.create("/does_not_exist"));
269+
270+
ActionOutputMetadataStore store = createStore(/* outputs= */ ImmutableSet.of(treeArtifact));
271+
272+
IOException e = assertThrows(IOException.class, () -> store.getOutputMetadata(treeArtifact));
273+
assertThat(e).hasMessageThat().contains("dangling symbolic link");
274+
}
275+
262276
@Test
263277
public void resettingOutputs() throws Exception {
264278
PathFragment path = PathFragment.create("foo/bar");

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -505,9 +505,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
505505

506506
ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
507507
assertThat(errors).hasSize(2);
508-
assertThat(errors.get(0).getMessage())
509-
.contains(
510-
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
508+
assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
511509
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
512510
}
513511

@@ -555,9 +553,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
555553

556554
ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
557555
assertThat(errors).hasSize(2);
558-
assertThat(errors.get(0).getMessage())
559-
.contains(
560-
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
556+
assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
561557
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
562558
}
563559

@@ -607,9 +603,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
607603

608604
ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
609605
assertThat(errors).hasSize(2);
610-
assertThat(errors.get(0).getMessage())
611-
.contains(
612-
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
606+
assertThat(errors.get(0).getMessage()).contains("child links/link is a dangling symbolic link");
613607
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
614608
}
615609

src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -419,9 +419,7 @@ public void visitTree_throwsOnDanglingSymlink() throws Exception {
419419
assertThrows(
420420
IOException.class,
421421
() -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
422-
assertThat(e)
423-
.hasMessageThat()
424-
.contains("Child symlink of tree artifact /tree is a dangling symbolic link");
422+
assertThat(e).hasMessageThat().contains("child symlink is a dangling symbolic link");
425423
}
426424

427425
@Test
@@ -456,9 +454,7 @@ public Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
456454
assertThrows(
457455
IOException.class,
458456
() -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
459-
assertThat(e)
460-
.hasMessageThat()
461-
.contains("Child unknown of tree artifact /tree has an unsupported type");
457+
assertThat(e).hasMessageThat().contains("child unknown has an unsupported type");
462458
}
463459

464460
@Test
@@ -523,9 +519,7 @@ public long getSize() {
523519
assertThrows(
524520
IOException.class,
525521
() -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
526-
assertThat(e)
527-
.hasMessageThat()
528-
.contains("Child sym of tree artifact /tree has an unsupported type");
522+
assertThat(e).hasMessageThat().contains("child sym has an unsupported type");
529523
}
530524

531525
@Test

0 commit comments

Comments
 (0)