[7.1.0] Follow directory symlink in RemoteActionFileSystem#getDirecto…

…ryEntries(). (#21088)

Although the semantics weren't explicitly specified by the FileSystem
interface, the Unix implementation calls readdir(3), which always
dereferences a symlink for the directory itself.

Also deduplicate the common logic.

PiperOrigin-RevId: 598543913
Change-Id: I9bcab70c57ef4e8c4da58f4871397c6f2362f43c

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

@@ -24,6 +24,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -63,6 +64,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.function.Function;
 import javax.annotation.Nullable;
 
 /**
@@ -767,56 +769,24 @@ public boolean createDirectory(PathFragment path) throws IOException {
   }
 
   @Override
-  protected ImmutableList<String> getDirectoryEntries(PathFragment path) throws IOException {
-    HashSet<String> entries = new HashSet<>();
-
-    boolean found = false;
-
-    if (path.startsWith(execRoot)) {
-      var execPath = path.relativeTo(execRoot);
-      Collection<Dirent> treeEntries = inputTreeArtifactDirectoryCache.get(execPath);
-      if (treeEntries != null) {
-        for (var entry : treeEntries) {
-          entries.add(entry.getName());
-        }
-        found = true;
-      }
-    }
-
-    if (isOutput(path)) {
-      try {
-        remoteOutputTree.getPath(path).getDirectoryEntries().stream()
-            .map(Path::getBaseName)
-            .forEach(entries::add);
-        found = true;
-      } catch (FileNotFoundException ignored) {
-        // Will be rethrown below if directory exists on neither side.
-      }
-    }
-
-    try {
-      localFs.getPath(path).getDirectoryEntries().stream()
-          .map(Path::getBaseName)
-          .forEach(entries::add);
-    } catch (FileNotFoundException e) {
-      if (!found) {
-        throw e;
-      }
-    }
-
-    // sort entries to get a deterministic order.
-    return ImmutableList.sortedCopyOf(entries);
+  protected Collection<String> getDirectoryEntries(PathFragment path) throws IOException {
+    return getDirectoryContents(path, /* followSymlinks= */ false, Dirent::getName);
   }
 
   @Override
   protected Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
       throws IOException {
-    HashMap<String, Dirent> entries = new HashMap<>();
-
-    boolean found = false;
+    return getDirectoryContents(path, followSymlinks, Function.identity());
+  }
 
+  private <T extends Comparable<T>> ImmutableSortedSet<T> getDirectoryContents(
+      PathFragment path, boolean followSymlinks, Function<Dirent, T> transformer)
+      throws IOException {
     path = resolveSymbolicLinks(path).asFragment();
 
+    HashMap<String, Dirent> entries = new HashMap<>();
+    boolean found = false;
+
     if (path.startsWith(execRoot)) {
       var execPath = path.relativeTo(execRoot);
       Collection<Dirent> treeEntries = inputTreeArtifactDirectoryCache.get(execPath);
@@ -851,8 +821,12 @@ protected Collection<Dirent> readdir(PathFragment path, boolean followSymlinks)
       }
     }
 
-    // sort entries to get a deterministic order.
-    return ImmutableList.sortedCopyOf(entries.values());
+    // Sort entries to get a deterministic order.
+    ImmutableSortedSet.Builder<T> builder = ImmutableSortedSet.naturalOrder();
+    for (var entry : entries.values()) {
+      builder.add(transformer.apply(entry));
+    }
+    return builder.build();
   }
 
   private Dirent maybeFollowSymlinkForDirent(

src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java

@@ -606,7 +606,7 @@ public boolean exists(PathFragment path) {
 
   /**
    * Returns a collection containing the names of all entities within the directory denoted by the
-   * {@code path}.
+   * {@code path}. Symlinks are followed when resolving the directory whose entries are to be read.
    *
    * @throws IOException if there was an error reading the directory entries
    */

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

@@ -676,7 +676,34 @@ public void readdir_fromMultipleSources() throws Exception {
   }
 
   @Test
-  public void readdir_followSymlinks(
+  public void readdir_followSymlinks_forDirectory(
+      @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
+      throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    FileSystem fromFs = from.getFilesystem(actionFs);
+    FileSystem toFs = to.getFilesystem(actionFs);
+
+    PathFragment linkPath = getOutputPath("sym");
+    PathFragment targetPath = getOutputPath("dir");
+    PathFragment childPath = getOutputPath("dir/child");
+
+    fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
+    toFs.getPath(targetPath).createDirectory();
+
+    if (toFs.equals(actionFs.getLocalFileSystem())) {
+      writeLocalFile(actionFs, childPath, "content");
+    } else {
+      injectRemoteFile(actionFs, childPath, "content");
+    }
+
+    assertReaddir(
+        actionFs, linkPath, /* followSymlinks= */ false, new Dirent("child", Dirent.Type.FILE));
+    assertReaddir(
+        actionFs, linkPath, /* followSymlinks= */ true, new Dirent("child", Dirent.Type.FILE));
+  }
+
+  @Test
+  public void readdir_followSymlinks_forDirectoryEntries(
       @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
       throws Exception {
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
@@ -707,6 +734,17 @@ public void readdir_followSymlinks(
         actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.FILE));
   }
 
+  @Test
+  public void readdir_nonDirectory() throws Exception {
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+    Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "dir/out");
+    PathFragment path = artifact.getPath().getParentDirectory().asFragment();
+
+    writeLocalFile(actionFs, path, "content");
+
+    assertReaddirThrows(actionFs, path, /* followSymlinks= */ true);
+  }
+
   @Test
   public void readdir_notFound() throws Exception {
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();