From 9c94ae4d7af839826c3b753d22af95ba90b20765 Mon Sep 17 00:00:00 2001 From: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Date: Thu, 10 Nov 2022 14:47:09 -0800 Subject: [PATCH] Implement getDirectoryEntries and readdir for RemoteActionFileSystem. (#16738) So spawns can read content of directires within action exuection. Part of https://github.com/bazelbuild/bazel/pull/16556. PiperOrigin-RevId: 486918859 Change-Id: Ida86e4c927093d26f7f96d2f0c2aa0d1d74cc8a4 Co-authored-by: Googler --- .../lib/remote/RemoteActionFileSystem.java | 81 ++++++++++-- .../RemoteActionFileSystemTestBase.java | 119 ++++++++++++++++++ 2 files changed, 189 insertions(+), 11 deletions(-) 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 4c352615c3336c..b7f31149f9f46f 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 @@ -22,6 +22,7 @@ import static com.google.common.collect.Streams.stream; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; @@ -50,6 +51,8 @@ import java.io.OutputStream; import java.nio.channels.ReadableByteChannel; import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import javax.annotation.Nullable; @@ -604,6 +607,73 @@ public boolean createDirectory(PathFragment path) throws IOException { return created; } + @Override + protected ImmutableList getDirectoryEntries(PathFragment path) throws IOException { + HashSet entries = new HashSet<>(); + + boolean ignoredNotFoundInRemote = false; + if (isOutput(path)) { + try { + delegateFs.getPath(path).getDirectoryEntries().stream() + .map(Path::getBaseName) + .forEach(entries::add); + ignoredNotFoundInRemote = true; + } catch (FileNotFoundException ignored) { + // Intentionally ignored + } + } + + try { + remoteOutputTree.getPath(path).getDirectoryEntries().stream() + .map(Path::getBaseName) + .forEach(entries::add); + } catch (FileNotFoundException e) { + if (!ignoredNotFoundInRemote) { + throw e; + } + } + + // sort entries to get a deterministic order. + return ImmutableList.sortedCopyOf(entries); + } + + @Override + protected Collection readdir(PathFragment path, boolean followSymlinks) + throws IOException { + HashMap entries = new HashMap<>(); + + boolean ignoredNotFoundInRemote = false; + if (isOutput(path)) { + try { + for (var entry : + delegateFs + .getPath(path) + .readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) { + entries.put(entry.getName(), entry); + } + ignoredNotFoundInRemote = true; + } catch (FileNotFoundException ignored) { + // Intentionally ignored + } + } + + try { + for (var entry : + remoteOutputTree + .getPath(path) + .readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) { + entries.put(entry.getName(), entry); + } + } catch (FileNotFoundException e) { + if (!ignoredNotFoundInRemote) { + throw e; + } + } + + // sort entries to get a deterministic order. + return ImmutableList.sortedCopyOf(entries.values()); + } + /* * -------------------- TODO(buchgr): Not yet implemented -------------------- * @@ -613,23 +683,12 @@ public boolean createDirectory(PathFragment path) throws IOException { * sure to fully implement this file system. */ - @Override - protected Collection getDirectoryEntries(PathFragment path) throws IOException { - return super.getDirectoryEntries(path); - } - @Override protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { super.createFSDependentHardLink(linkPath, originalPath); } - @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) - throws IOException { - return super.readdir(path, followSymlinks); - } - @Override protected void createHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java index fd8a82d80cda6c..a39e326175062b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java @@ -13,14 +13,18 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; import java.io.FileNotFoundException; import java.io.IOException; import org.junit.Test; @@ -209,4 +213,119 @@ public void createDirectory_createLocallyAndRemotely() throws Exception { assertThat(getRemoteFileSystem(actionFs).getPath(path).isDirectory()).isTrue(); assertThat(getLocalFileSystem(actionFs).getPath(path).isDirectory()).isTrue(); } + + interface DirectoryEntriesProvider { + ImmutableList getDirectoryEntries(Path path) throws IOException; + } + + private void readdirNonEmptyLocalDirectoryReadFromLocal( + DirectoryEntriesProvider directoryEntriesProvider) throws IOException { + FileSystem actionFs = createActionFileSystem(); + PathFragment dir = getOutputPath("parent/dir"); + actionFs.getPath(dir).createDirectoryAndParents(); + writeLocalFile(actionFs, dir.getChild("file1"), "content1"); + writeLocalFile(actionFs, dir.getChild("file2"), "content2"); + + ImmutableList entries = + directoryEntriesProvider.getDirectoryEntries(actionFs.getPath(dir)); + + assertThat(entries).containsExactly("file1", "file2"); + } + + private void readdirNonEmptyInMemoryDirectoryReadFromMemory( + DirectoryEntriesProvider directoryEntriesProvider) throws IOException { + FileSystem actionFs = createActionFileSystem(); + PathFragment dir = getOutputPath("parent/dir"); + actionFs.getPath(dir).createDirectoryAndParents(); + injectRemoteFile(actionFs, dir.getChild("file1"), "content1"); + injectRemoteFile(actionFs, dir.getChild("file2"), "content2"); + + ImmutableList entries = + directoryEntriesProvider.getDirectoryEntries(actionFs.getPath(dir)); + + assertThat(entries).containsExactly("file1", "file2"); + } + + private void readdirNonEmptyLocalAndInMemoryDirectoryCombineThem( + DirectoryEntriesProvider directoryEntriesProvider) throws IOException { + FileSystem actionFs = createActionFileSystem(); + PathFragment dir = getOutputPath("parent/dir"); + actionFs.getPath(dir).createDirectoryAndParents(); + writeLocalFile(actionFs, dir.getChild("file1"), "content1"); + writeLocalFile(actionFs, dir.getChild("file2"), "content2"); + injectRemoteFile(actionFs, dir.getChild("file2"), "content2inmemory"); + injectRemoteFile(actionFs, dir.getChild("file3"), "content3"); + + ImmutableList entries = + directoryEntriesProvider.getDirectoryEntries(actionFs.getPath(dir)); + + assertThat(entries).containsExactly("file1", "file2", "file3"); + } + + private void readdirNothingThereThrowsFileNotFound( + DirectoryEntriesProvider directoryEntriesProvider) throws IOException { + FileSystem actionFs = createActionFileSystem(); + PathFragment dir = getOutputPath("parent/dir"); + + assertThrows( + FileNotFoundException.class, + () -> directoryEntriesProvider.getDirectoryEntries(actionFs.getPath(dir))); + } + + @Test + public void readdir_nonEmptyLocalDirectory_readFromLocal() throws IOException { + readdirNonEmptyLocalDirectoryReadFromLocal( + path -> + path.readdir(Symlinks.FOLLOW).stream().map(Dirent::getName).collect(toImmutableList())); + } + + @Test + public void readdir_nonEmptyInMemoryDirectory_readFromMemory() throws IOException { + readdirNonEmptyInMemoryDirectoryReadFromMemory( + path -> + path.readdir(Symlinks.FOLLOW).stream().map(Dirent::getName).collect(toImmutableList())); + } + + @Test + public void readdir_nonEmptyLocalAndInMemoryDirectory_combineThem() throws IOException { + readdirNonEmptyLocalAndInMemoryDirectoryCombineThem( + path -> + path.readdir(Symlinks.FOLLOW).stream().map(Dirent::getName).collect(toImmutableList())); + } + + @Test + public void readdir_nothingThere_throwsFileNotFound() throws IOException { + readdirNothingThereThrowsFileNotFound( + path -> + path.readdir(Symlinks.FOLLOW).stream().map(Dirent::getName).collect(toImmutableList())); + } + + @Test + public void getDirectoryEntries_nonEmptyLocalDirectory_readFromLocal() throws IOException { + readdirNonEmptyLocalDirectoryReadFromLocal( + path -> + path.getDirectoryEntries().stream().map(Path::getBaseName).collect(toImmutableList())); + } + + @Test + public void getDirectoryEntries_nonEmptyInMemoryDirectory_readFromMemory() throws IOException { + readdirNonEmptyInMemoryDirectoryReadFromMemory( + path -> + path.getDirectoryEntries().stream().map(Path::getBaseName).collect(toImmutableList())); + } + + @Test + public void getDirectoryEntries_nonEmptyLocalAndInMemoryDirectory_combineThem() + throws IOException { + readdirNonEmptyLocalAndInMemoryDirectoryCombineThem( + path -> + path.getDirectoryEntries().stream().map(Path::getBaseName).collect(toImmutableList())); + } + + @Test + public void getDirectoryEntries_nothingThere_throwsFileNotFound() throws IOException { + readdirNothingThereThrowsFileNotFound( + path -> + path.getDirectoryEntries().stream().map(Path::getBaseName).collect(toImmutableList())); + } }