Skip to content

Commit

Permalink
Implement getDirectoryEntries and readdir for RemoteActionFileSystem.
Browse files Browse the repository at this point in the history 
So spawns can read content of directires within action exuection.

Part of #16556.

PiperOrigin-RevId: 486918859
Change-Id: Ida86e4c927093d26f7f96d2f0c2aa0d1d74cc8a4
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Nov 8, 2022
1 parent a7fc88a commit 97dea59
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 11 deletions.
81 changes: 70 additions & 11 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -604,6 +607,73 @@ public boolean createDirectory(PathFragment path) throws IOException {
return created;
}

@Override
protected ImmutableList<String> getDirectoryEntries(PathFragment path) throws IOException {
HashSet<String> 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<Dirent> readdir(PathFragment path, boolean followSymlinks)
throws IOException {
HashMap<String, Dirent> 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 --------------------
*
Expand All @@ -613,23 +683,12 @@ public boolean createDirectory(PathFragment path) throws IOException {
* sure to fully implement this file system.
*/

@Override
protected Collection<String> 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<Dirent> readdir(PathFragment path, boolean followSymlinks)
throws IOException {
return super.readdir(path, followSymlinks);
}

@Override
protected void createHardLink(PathFragment linkPath, PathFragment originalPath)
throws IOException {
Expand Down
119 changes: 119 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> 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<String> 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<String> 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<String> 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()));
}
}

2 comments on commit 97dea59

@coeuvre
Copy link
Member Author

@coeuvre coeuvre commented on 97dea59 Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgowroji Please cherry-pick this commit into 6.0 if that is possible, thanks!

@ShreeM01
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @coeuvre! Here is the cherry-picked PR #16700. Could you please review it. Thanks!

Please sign in to comment.