diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
index 13697b03fcfc4d..5aac578fe3cd33 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
@@ -283,10 +283,6 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
return TreeArtifactValue.MISSING_TREE_ARTIFACT;
}
- if (chmod) {
- setPathPermissions(treeDir);
- }
-
AtomicBoolean anyRemote = new AtomicBoolean(false);
TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
index 0604e6a6beb34e..d2f8df1839ede6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -518,7 +518,7 @@ static class Visitor extends AbstractQueueVisitor {
}
void run() throws IOException, InterruptedException {
- execute(() -> visitTree(PathFragment.EMPTY_FRAGMENT));
+ execute(() -> visit(PathFragment.EMPTY_FRAGMENT, Dirent.Type.DIRECTORY));
try {
awaitQuiescence(true);
} catch (IORuntimeException e) {
@@ -526,30 +526,25 @@ void run() throws IOException, InterruptedException {
}
}
- // IOExceptions are wrapped in IORuntimeException so that it can be propagated to the main
- // thread
- private void visitTree(PathFragment subdir) {
+ private void visit(PathFragment parentRelativePath, Dirent.Type type) {
try {
- for (Dirent dirent : parentDir.getRelative(subdir).readdir(Symlinks.NOFOLLOW)) {
- PathFragment parentRelativePath = subdir.getChild(dirent.getName());
- Dirent.Type type = dirent.getType();
-
- if (type == Dirent.Type.UNKNOWN) {
- throw new IOException(
- "Could not determine type of file for "
- + parentRelativePath
- + " under "
- + parentDir);
- }
+ Path path = parentDir.getRelative(parentRelativePath);
- if (type == Dirent.Type.SYMLINK) {
- checkSymlink(subdir, parentDir.getRelative(parentRelativePath));
- }
+ if (type == Dirent.Type.UNKNOWN) {
+ throw new IOException("Could not determine type of file for " + path.getPathString());
+ }
+
+ if (type == Dirent.Type.SYMLINK) {
+ checkSymlink(parentRelativePath.getParentDirectory(), path);
+ }
- visitor.visit(parentRelativePath, type);
+ visitor.visit(parentRelativePath, type);
- if (type == Dirent.Type.DIRECTORY) {
- execute(() -> visitTree(parentRelativePath));
+ if (type == Dirent.Type.DIRECTORY) {
+ for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) {
+ PathFragment childPath = parentRelativePath.getChild(dirent.getName());
+ Dirent.Type childType = dirent.getType();
+ execute(() -> visit(childPath, childType));
}
}
} catch (IOException e) {
@@ -563,7 +558,7 @@ private void visitTree(PathFragment subdir) {
* Recursively visits all descendants under a directory.
*
* {@link TreeArtifactVisitor#visit} is invoked on {@code visitor} for each directory, file,
- * and symlink under the given {@code parentDir}.
+ * and symlink under the given {@code parentDir}, including {@code parentDir} itself.
*
*
This method is intended to provide uniform semantics for constructing a tree artifact,
* including special logic that validates directory entries. Invalid directory entries include a
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
index ce6dac4ad0af46..53ca1c98bf6820 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -15,7 +15,6 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.fail;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;
@@ -382,6 +381,7 @@ public void visitTree_visitsEachChild() throws Exception {
assertThat(children)
.containsExactly(
+ Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a/b"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("file1"), Dirent.Type.FILE),
@@ -405,17 +405,13 @@ public ImmutableList readdir(PathFragment path, boolean followSymlinks)
Exception e =
assertThrows(
- IOException.class,
- () ->
- TreeArtifactValue.visitTree(
- treeDir, (child, type) -> fail("Should not be called")));
- assertThat(e).hasMessageThat().contains("Could not determine type of file for ? under /tree");
+ IOException.class, () -> TreeArtifactValue.visitTree(treeDir, (child, type) -> {}));
+ assertThat(e).hasMessageThat().contains("Could not determine type of file for /tree/?");
}
@Test
public void visitTree_propagatesIoExceptionFromVisitor() throws Exception {
Path treeDir = scratch.dir("tree");
- scratch.file("tree/file");
IOException e = new IOException("From visitor");
IOException thrown =
@@ -425,8 +421,6 @@ public void visitTree_propagatesIoExceptionFromVisitor() throws Exception {
TreeArtifactValue.visitTree(
treeDir,
(child, type) -> {
- assertThat(child).isEqualTo(PathFragment.create("file"));
- assertThat(type).isEqualTo(Dirent.Type.FILE);
throw e;
}));
assertThat(thrown).isSameInstanceAs(e);
@@ -450,6 +444,7 @@ public void visitTree_pemitsUpLevelSymlinkInsideTree() throws Exception {
assertThat(children)
.containsExactly(
+ Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("file"), Dirent.Type.FILE),
Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a/up_link"), Dirent.Type.SYMLINK));
@@ -470,7 +465,9 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception {
});
assertThat(children)
- .containsExactly(Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
+ .containsExactly(
+ Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
+ Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
}
@Test
@@ -478,13 +475,20 @@ public void visitTree_throwsOnSymlinkPointingOutsideTree() throws Exception {
Path treeDir = scratch.dir("tree");
scratch.file("outside");
scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../outside"));
+ List> children = new ArrayList<>();
Exception e =
assertThrows(
IOException.class,
() ->
TreeArtifactValue.visitTree(
- treeDir, (child, type) -> fail("Should not be called")));
+ treeDir,
+ (child, type) -> {
+ synchronized (children) {
+ children.add(Pair.of(child, type));
+ }
+ }));
+ assertThat(children).containsExactly(Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY));
assertThat(e).hasMessageThat().contains("/tree/link pointing to ../outside");
}
@@ -493,6 +497,7 @@ public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throw
Path treeDir = scratch.dir("tree");
scratch.file("tree/file");
scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../tree/file"));
+ List> children = new ArrayList<>();
Exception e =
assertThrows(
@@ -501,9 +506,14 @@ public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throw
TreeArtifactValue.visitTree(
treeDir,
(child, type) -> {
- assertThat(child).isEqualTo(PathFragment.create("file"));
- assertThat(type).isEqualTo(Dirent.Type.FILE);
+ synchronized (children) {
+ children.add(Pair.of(child, type));
+ }
}));
+ assertThat(children)
+ .containsExactly(
+ Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
+ Pair.of(PathFragment.create("file"), Dirent.Type.FILE));
assertThat(e).hasMessageThat().contains("/tree/link pointing to ../tree/file");
}