diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 4786b36ad272d4..5f78a98fc5d1d4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -895,9 +895,6 @@ private Dirent maybeFollowSymlinkForDirent( } PathFragment path = dirPath.getChild(entry.getName()); FileStatus st = statNullable(path, /* followSymlinks= */ true); - if (st == null) { - return new Dirent(entry.getName(), Dirent.Type.UNKNOWN); - } return new Dirent(entry.getName(), direntFromStat(st)); } diff --git a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java index cf8e2fb99c0cf5..d29e63739331c1 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java +++ b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java @@ -255,15 +255,23 @@ private static Type forChar(char c) { /** The names of the entries in a directory. */ private final String[] names; + /** - * An optional (nullable) array of entry types, corresponding positionally - * to the "names" field. The types are: - * 'd': a subdirectory - * 'f': a regular file - * 's': a symlink (only returned with {@code NOFOLLOW}) - * '?': anything else - * Note that unlike libc, this implementation of readdir() follows - * symlinks when determining these types. + * An optional (nullable) array of entry types, corresponding positionally to the "names" field. + * The possible types are: + * + * * *

This is intentionally a byte array rather than a array of enums to save memory. */ diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Dirent.java b/src/main/java/com/google/devtools/build/lib/vfs/Dirent.java index f3fbe0692f2b46..cd145937e2dcfa 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Dirent.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Dirent.java @@ -26,7 +26,8 @@ public enum Type { DIRECTORY, // A symlink. SYMLINK, - // Not one of the above. For example, a special file. + // None of the above. + // For example, a special file, or a path that could not be resolved while following symlinks. UNKNOWN; } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index 85b017b061442c..aae001bab45914 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java @@ -618,7 +618,7 @@ public boolean exists(PathFragment path) { */ protected abstract Collection getDirectoryEntries(PathFragment path) throws IOException; - protected static Dirent.Type direntFromStat(FileStatus stat) { + protected static Dirent.Type direntFromStat(@Nullable FileStatus stat) { if (stat == null) { return Dirent.Type.UNKNOWN; } else if (stat.isSpecialFile()) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index fe2af660aac9b9..f2bbc4c99b05e3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -675,17 +675,15 @@ public void readdir_fromRemoteOutputTree() throws Exception { @Test public void readdir_fromLocalFilesystem() throws Exception { RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); - writeLocalFile(actionFs, getOutputPath("dir/out1"), "contents1"); - writeLocalFile(actionFs, getOutputPath("dir/out2"), "contents2"); - writeLocalFile(actionFs, getOutputPath("dir/subdir/out3"), "contents3"); + writeLocalFile(actionFs, getOutputPath("dir/file"), "contents"); + writeLocalFile(actionFs, getOutputPath("dir/subdir/file"), "contents"); PathFragment dirPath = getOutputPath("dir"); assertReaddir( actionFs, dirPath, /* followSymlinks= */ true, - new Dirent("out1", Dirent.Type.FILE), - new Dirent("out2", Dirent.Type.FILE), + new Dirent("file", Dirent.Type.FILE), new Dirent("subdir", Dirent.Type.DIRECTORY)); assertReaddirThrows(actionFs, getOutputPath("dir/out1"), /* followSymlinks= */ true); @@ -792,27 +790,49 @@ public void readdir_followSymlinks_forDirectoryEntries( FileSystem toFs = to.getFilesystem(actionFs); PathFragment dirPath = getOutputPath("dir"); - PathFragment linkPath = getOutputPath("dir/sym"); - PathFragment targetPath = getOutputPath("target"); + PathFragment fileLinkPath = getOutputPath("dir/file_sym"); + PathFragment fileTargetPath = getOutputPath("file_target"); + PathFragment dirLinkPath = getOutputPath("dir/dir_sym"); + PathFragment dirTargetPath = getOutputPath("dir_target"); + PathFragment loopingLinkPath = getOutputPath("dir/looping_sym"); + PathFragment danglingLinkPath = getOutputPath("dir/dangling_sym"); fromFs.getPath(dirPath).createDirectory(); - fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment()); - - assertReaddir( - actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK)); - assertReaddir( - actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.UNKNOWN)); + fromFs + .getPath(fileLinkPath) + .createSymbolicLink(execRoot.getRelative(fileTargetPath).asFragment()); + fromFs + .getPath(dirLinkPath) + .createSymbolicLink(execRoot.getRelative(dirTargetPath).asFragment()); + fromFs + .getPath(loopingLinkPath) + .createSymbolicLink(execRoot.getRelative(loopingLinkPath).asFragment()); + fromFs.getPath(danglingLinkPath).createSymbolicLink(PathFragment.create("/does_not_exist")); if (toFs.equals(actionFs.getLocalFileSystem())) { - writeLocalFile(actionFs, targetPath, "content"); + writeLocalFile(actionFs, fileTargetPath, "content"); + actionFs.getLocalFileSystem().getPath(dirTargetPath).createDirectoryAndParents(); } else { - injectRemoteFile(actionFs, targetPath, "content"); + injectRemoteFile(actionFs, fileTargetPath, "content"); + actionFs.getRemoteOutputTree().createDirectoryAndParents(dirTargetPath); } assertReaddir( - actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK)); + actionFs, + dirPath, + /* followSymlinks= */ false, + new Dirent("file_sym", Dirent.Type.SYMLINK), + new Dirent("dir_sym", Dirent.Type.SYMLINK), + new Dirent("looping_sym", Dirent.Type.SYMLINK), + new Dirent("dangling_sym", Dirent.Type.SYMLINK)); assertReaddir( - actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.FILE)); + actionFs, + dirPath, + /* followSymlinks= */ true, + new Dirent("file_sym", Dirent.Type.FILE), + new Dirent("dir_sym", Dirent.Type.DIRECTORY), + new Dirent("looping_sym", Dirent.Type.UNKNOWN), + new Dirent("dangling_sym", Dirent.Type.UNKNOWN)); } @Test @@ -841,12 +861,10 @@ private void assertReaddir( boolean followSymlinks, Dirent... expected) throws Exception { - assertThat(actionFs.readdir(dirPath, followSymlinks)) - .containsExactlyElementsIn(expected) - .inOrder(); + assertThat(actionFs.readdir(dirPath, followSymlinks)).containsExactlyElementsIn(expected); assertThat(actionFs.getDirectoryEntries(dirPath)) - .containsExactlyElementsIn(stream(expected).map(Dirent::getName).collect(toImmutableList())) - .inOrder(); + .containsExactlyElementsIn( + stream(expected).map(Dirent::getName).collect(toImmutableList())); } private void assertReaddirThrows( diff --git a/src/test/java/com/google/devtools/build/lib/unix/UnixFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/unix/UnixFileSystemTest.java index 2c772632bb672a..732b76b5eb6158 100644 --- a/src/test/java/com/google/devtools/build/lib/unix/UnixFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/unix/UnixFileSystemTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.assertThrows; import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -61,6 +62,7 @@ public void testIsSpecialFile() throws Exception { Path fifo = absolutize("fifo"); FileSystemUtils.createEmptyFile(regular); NativePosixFiles.mkfifo(fifo.toString(), 0777); + assertThat(regular.isFile()).isTrue(); assertThat(regular.isSpecialFile()).isFalse(); assertThat(regular.stat().isFile()).isTrue(); @@ -70,4 +72,24 @@ public void testIsSpecialFile() throws Exception { assertThat(fifo.stat().isFile()).isTrue(); assertThat(fifo.stat().isSpecialFile()).isTrue(); } + + @Test + public void testReaddirSpecialFile() throws Exception { + Path dir = absolutize("readdir"); + Path symlink = dir.getChild("symlink"); + Path fifo = dir.getChild("fifo"); + dir.createDirectoryAndParents(); + symlink.createSymbolicLink(fifo.asFragment()); + NativePosixFiles.mkfifo(fifo.toString(), 0777); + + assertThat(dir.getDirectoryEntries()).containsExactly(symlink, fifo); + + assertThat(dir.readdir(Symlinks.NOFOLLOW)) + .containsExactly( + new Dirent("symlink", Dirent.Type.SYMLINK), new Dirent("fifo", Dirent.Type.UNKNOWN)); + + assertThat(dir.readdir(Symlinks.FOLLOW)) + .containsExactly( + new Dirent("symlink", Dirent.Type.UNKNOWN), new Dirent("fifo", Dirent.Type.UNKNOWN)); + } } diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java index 6316a49ea89bce..382502351f2934 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java @@ -20,6 +20,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -1768,29 +1769,68 @@ public void testStatNullableFailsFastOnNonExistingFiles() throws Exception { @Test public void testResolveSymlinks() throws Exception { - if (testFS.supportsSymbolicLinksNatively(xLink.asFragment())) { - createSymbolicLink(xLink, xFile); - FileSystemUtils.createEmptyFile(xFile); - assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xFile.asFragment()); - assertThat(xLink.resolveSymbolicLinks()).isEqualTo(xFile); - } + assumeTrue(testFS.supportsSymbolicLinksNatively(xLink.asFragment())); + + createSymbolicLink(xLink, xFile); + FileSystemUtils.createEmptyFile(xFile); + assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xFile.asFragment()); + assertThat(xLink.resolveSymbolicLinks()).isEqualTo(xFile); } @Test public void testResolveDanglingSymlinks() throws Exception { - if (testFS.supportsSymbolicLinksNatively(xLink.asFragment())) { - createSymbolicLink(xLink, xNothing); - assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xNothing.asFragment()); - assertThrows(IOException.class, () -> xLink.resolveSymbolicLinks()); - } + assumeTrue(testFS.supportsSymbolicLinksNatively(xLink.asFragment())); + + createSymbolicLink(xLink, xNothing); + assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xNothing.asFragment()); + assertThrows(IOException.class, () -> xLink.resolveSymbolicLinks()); } @Test public void testResolveNonSymlinks() throws Exception { - if (testFS.supportsSymbolicLinksNatively(xFile.asFragment())) { - assertThat(testFS.resolveOneLink(xFile.asFragment())).isNull(); - assertThat(xFile.resolveSymbolicLinks()).isEqualTo(xFile); - } + assertThat(testFS.resolveOneLink(xFile.asFragment())).isNull(); + assertThat(xFile.resolveSymbolicLinks()).isEqualTo(xFile); + } + + @Test + public void testReaddir() throws Exception { + Path dir = workingDir.getChild("readdir"); + + assumeTrue(testFS.supportsSymbolicLinksNatively(dir.asFragment())); + + dir.getChild("dir").createDirectoryAndParents(); + FileSystemUtils.createEmptyFile(dir.getChild("file")); + dir.getChild("file_link").createSymbolicLink(dir.getChild("file")); + dir.getChild("dir_link").createSymbolicLink(dir.getChild("dir")); + dir.getChild("looping_link").createSymbolicLink(dir.getChild("looping_link")); + dir.getChild("dangling_link").createSymbolicLink(testFS.getPath("/does_not_exist")); + + assertThat(dir.getDirectoryEntries()) + .containsExactly( + dir.getChild("file"), + dir.getChild("dir"), + dir.getChild("file_link"), + dir.getChild("dir_link"), + dir.getChild("looping_link"), + dir.getChild("dangling_link")); + + assertThat(dir.readdir(Symlinks.NOFOLLOW)) + .containsExactly( + new Dirent("file", Dirent.Type.FILE), + new Dirent("dir", Dirent.Type.DIRECTORY), + new Dirent("file_link", Dirent.Type.SYMLINK), + new Dirent("dir_link", Dirent.Type.SYMLINK), + new Dirent("looping_link", Dirent.Type.SYMLINK), + new Dirent("dangling_link", Dirent.Type.SYMLINK)); + + assertThat(dir.readdir(Symlinks.FOLLOW)) + .containsExactly( + new Dirent("file", Dirent.Type.FILE), + new Dirent("dir", Dirent.Type.DIRECTORY), + new Dirent("file_link", Dirent.Type.FILE), + new Dirent("dir_link", Dirent.Type.DIRECTORY), + new Dirent("looping_link", Dirent.Type.UNKNOWN), + new Dirent("dangling_link", Dirent.Type.UNKNOWN)); } @Test