From 3add4cc684e36b6ae9e8b04ea0400afe32cb20be Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 22 Aug 2025 08:50:54 -0700 Subject: [PATCH 1/5] Make non-final methods in `FileSystem` public Makes it possible to build `FileSystem`s composed out of existing `FileSystem`s outside the `vfs` package. Some methods already had their visibility upgraded for this purpose. Closes #26796. PiperOrigin-RevId: 798226663 Change-Id: Ic9e4cdefadc7ad8d50a5b69df91837c99c5f6c64 (cherry picked from commit 4f46e8265313078b8afbf9d6b832711b2d75cfc2) --- .../analysis/ConfiguredRuleClassProvider.java | 4 +- .../lib/remote/RemoteActionFileSystem.java | 65 ++++++-------- .../lib/testing/vfs/SpiedFileSystem.java | 27 ------ .../build/lib/unix/UnixFileSystem.java | 51 ++++++----- .../build/lib/vfs/AbstractFileSystem.java | 6 +- .../vfs/AbstractFileSystemWithCustomStat.java | 10 +-- .../devtools/build/lib/vfs/FileSystem.java | 85 +++++++++---------- .../build/lib/vfs/JavaIoFileSystem.java | 44 +++++----- .../PathTransformingDelegateFileSystem.java | 79 +++++++++-------- .../vfs/ReadonlyFileSystemWithCustomStat.java | 13 ++- .../vfs/inmemoryfs/InMemoryFileSystem.java | 40 +++++---- .../build/lib/windows/WindowsFileSystem.java | 16 ++-- .../analysis/InterruptedExceptionTest.java | 2 +- .../analysis/StubbableFSBuildViewTest.java | 2 +- .../lib/buildtool/ProgressReportingTest.java | 4 +- .../lib/exec/SingleBuildFileCacheTest.java | 6 +- .../lib/exec/SpawnLogContextTestBase.java | 4 +- .../lib/pkgcache/LoadingPhaseRunnerTest.java | 2 +- .../TargetPatternEvaluatorIOTest.java | 2 +- .../build/lib/sandbox/SandboxHelpersTest.java | 2 +- .../skyframe/ArtifactFunctionTestCase.java | 2 +- .../lib/skyframe/BzlLoadFunctionTest.java | 2 +- .../lib/skyframe/FileArtifactValueTest.java | 2 +- .../build/lib/skyframe/FileFunctionTest.java | 10 +-- .../LocalDiffAwarenessIntegrationTest.java | 3 +- .../lib/skyframe/PackageFunctionTest.java | 2 +- ...psOfTargetsUnderDirectoryFunctionTest.java | 3 +- ...ursiveFilesystemTraversalFunctionTest.java | 3 +- .../unix/UnixDigestHashAttributeNameTest.java | 2 +- .../build/lib/vfs/DigestUtilsTest.java | 8 +- 30 files changed, 226 insertions(+), 275 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index c4f56e920e8110..bf8e838663a07f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -126,12 +126,12 @@ public BundledFileSystem() { // See b/226379109 for details. @Override - protected synchronized byte[] getFastDigest(PathFragment path) { + public synchronized byte[] getFastDigest(PathFragment path) { return getDigest(path); } @Override - protected synchronized byte[] getDigest(PathFragment path) { + public synchronized byte[] getDigest(PathFragment path) { return getDigestFunction() .getHashFunction() .hashBytes(StringUnsafe.getInternalStringBytes(path.getPathString())) 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 b8e39b245f20da..c516de5dc12369 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 @@ -190,8 +190,8 @@ public long getNodeId() { /** * Caches the contents of intermediate subdirectories of tree artifact inputs, to speed up {@link - * #stat} and {@link #readdir} operations. Note that actions are not expected to modify their - * inputs. + * FileSystem#stat} and {@link FileSystem#readdir} operations. Note that actions are not expected + * to modify their inputs. * *

Safe for concurrent access. */ @@ -340,7 +340,7 @@ public String getFileSystemType(PathFragment path) { } @Override - protected Path resolveSymbolicLinks(PathFragment path) throws IOException { + public Path resolveSymbolicLinks(PathFragment path) throws IOException { return getPath(pathCanonicalizer.resolveSymbolicLinks(path)); } @@ -370,7 +370,7 @@ private PathFragment resolveSymbolicLinksForParent(PathFragment path) throws IOE } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { try { path = resolveSymbolicLinksForParent(path); } catch (FileNotFoundException ignored) { @@ -391,7 +391,7 @@ protected boolean delete(PathFragment path) throws IOException { } @Override - protected InputStream getInputStream(PathFragment path) throws IOException { + public InputStream getInputStream(PathFragment path) throws IOException { try { downloadFileIfRemote(path); } catch (BulkTransferException e) { @@ -408,13 +408,13 @@ protected InputStream getInputStream(PathFragment path) throws IOException { } @Override - protected OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) throws IOException { return localFs.getPath(path).getOutputStream(append, internal); } @Override - protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { + public SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { return localFs.getPath(path).createReadWriteByteChannel(); } @@ -455,7 +455,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) @Override @Nullable - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); // Try to obtain a fast digest through a stat. This is only possible for in-memory files. // The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is @@ -468,7 +468,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException { } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); // Try to obtain a fast digest through a stat. This is only possible for in-memory files. // The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is @@ -481,7 +481,7 @@ protected byte[] getDigest(PathFragment path) throws IOException { } @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { return localFs.getPath(path).isReadable(); @@ -492,7 +492,7 @@ protected boolean isReadable(PathFragment path) throws IOException { } @Override - protected boolean isWritable(PathFragment path) throws IOException { + public boolean isWritable(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { return localFs.getPath(path).isWritable(); @@ -503,7 +503,7 @@ protected boolean isWritable(PathFragment path) throws IOException { } @Override - protected boolean isExecutable(PathFragment path) throws IOException { + public boolean isExecutable(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { return localFs.getPath(path).isExecutable(); @@ -514,7 +514,7 @@ protected boolean isExecutable(PathFragment path) throws IOException { } @Override - protected void setReadable(PathFragment path, boolean readable) throws IOException { + public void setReadable(PathFragment path, boolean readable) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { localFs.getPath(path).setReadable(readable); @@ -534,7 +534,7 @@ public void setWritable(PathFragment path, boolean writable) throws IOException } @Override - protected void setExecutable(PathFragment path, boolean executable) throws IOException { + public void setExecutable(PathFragment path, boolean executable) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { localFs.getPath(path).setExecutable(executable); @@ -544,7 +544,7 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc } @Override - protected void chmod(PathFragment path, int mode) throws IOException { + public void chmod(PathFragment path, int mode) throws IOException { path = resolveSymbolicLinks(path).asFragment(); try { localFs.getPath(path).chmod(mode); @@ -554,7 +554,7 @@ protected void chmod(PathFragment path, int mode) throws IOException { } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { path = resolveSymbolicLinksForParent(path); if (path.startsWith(execRoot)) { @@ -585,7 +585,7 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { linkPath = resolveSymbolicLinksForParent(linkPath); @@ -598,19 +598,19 @@ protected void createSymbolicLink( } @Override - protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { FileStatus stat = stat(path, followSymlinks); return stat.getLastModifiedTime(); } @Override - protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { FileStatus stat = stat(path, followSymlinks); return stat.getSize(); } @Override - protected boolean exists(PathFragment path, boolean followSymlinks) { + public boolean exists(PathFragment path, boolean followSymlinks) { try { return statIfFound(path, followSymlinks) != null; } catch (IOException e) { @@ -619,7 +619,7 @@ protected boolean exists(PathFragment path, boolean followSymlinks) { } @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { FileStatus stat = statIfFound(path, followSymlinks); if (stat == null) { throw new FileNotFoundException(path.getPathString() + " (No such file or directory)"); @@ -629,14 +629,14 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx @Nullable @Override - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { return statInternal( path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT, StatSources.ALL); } @Nullable @Override - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { try { return statIfFound(path, followSymlinks); } catch (IOException e) { @@ -862,13 +862,12 @@ public boolean createDirectory(PathFragment path) throws IOException { } @Override - protected Collection getDirectoryEntries(PathFragment path) throws IOException { + public Collection getDirectoryEntries(PathFragment path) throws IOException { return getDirectoryContents(path, /* followSymlinks= */ false, Dirent::getName); } @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) - throws IOException { + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { return getDirectoryContents(path, followSymlinks, Function.identity()); } @@ -940,15 +939,14 @@ private Dirent maybeFollowSymlinkForDirent( } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { // Only called by the AbstractFileSystem#createHardLink base implementation, overridden below. throw new UnsupportedOperationException(); } @Override - protected void createHardLink(PathFragment linkPath, PathFragment originalPath) - throws IOException { + public void createHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { localFs.getPath(linkPath).createHardLink(getPath(originalPath)); } @@ -971,7 +969,7 @@ public RemoteInMemoryFileSystem(DigestHashFunction hashFunction) { } @Override - protected synchronized OutputStream getOutputStream( + public synchronized OutputStream getOutputStream( PathFragment path, boolean append, boolean internal) throws IOException { // To get an output stream from remote file, we need to first stage it. throw new IllegalStateException("Shouldn't be called directly"); @@ -993,13 +991,6 @@ protected void injectFile(PathFragment path, FileArtifactValue metadata) throws remoteInMemoryFileInfo.set(metadata); } - - // Override for access within this class - @Nullable - @Override - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { - return super.statNullable(path, followSymlinks); - } } static class RemoteInMemoryFileInfo extends FileInfo implements FileStatusWithMetadata { diff --git a/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java b/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java index 3487cf6f47118f..37007a5c9bc176 100644 --- a/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java @@ -18,16 +18,10 @@ import com.google.devtools.build.lib.vfs.DelegateFileSystem; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; -import java.io.IOException; -import java.io.OutputStream; /** * Delegate file system with the sole purpose of creating a {@link org.mockito.Mockito#spy}. - * - *

This class purposely makes the {@link FileSystem} methods public so that tests can mock those. - * Please feel free to add any methods you need. */ public class SpiedFileSystem extends DelegateFileSystem { @@ -45,25 +39,4 @@ public static SpiedFileSystem createSpy(FileSystem fileSystem) { public static SpiedFileSystem createInMemorySpy() { return createSpy(new InMemoryFileSystem(DigestHashFunction.SHA256)); } - - @Override - public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) - throws IOException { - return super.getOutputStream(path, append, internal); - } - - @Override - public boolean createWritableDirectory(PathFragment path) throws IOException { - return super.createWritableDirectory(path); - } - - @Override - public byte[] getDigest(PathFragment path) throws IOException { - return super.getDigest(path); - } - - @Override - public void chmod(PathFragment path, int mode) throws IOException { - super.chmod(path, mode); - } } diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java index 38627f4b893ca8..3b54f94bfb413c 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java @@ -133,7 +133,7 @@ public String toString() { } @Override - protected Collection getDirectoryEntries(PathFragment path) throws IOException { + public Collection getDirectoryEntries(PathFragment path) throws IOException { String name = path.getPathString(); String[] entries; long startTime = Profiler.nanoTimeMaybe(); @@ -147,7 +147,7 @@ protected Collection getDirectoryEntries(PathFragment path) throws IOExc @Override @Nullable - protected PathFragment resolveOneLink(PathFragment path) throws IOException { + public PathFragment resolveOneLink(PathFragment path) throws IOException { // Beware, this seemingly simple code belies the complex specification of // FileSystem.resolveOneLink(). return stat(path, false).isSymbolicLink() ? readSymbolicLink(path) : null; @@ -173,8 +173,7 @@ private static Dirent.Type convertToDirentType(Dirents.Type type) { } @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) - throws IOException { + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { String name = path.getPathString(); long startTime = Profiler.nanoTimeMaybe(); try { @@ -193,7 +192,7 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) } @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { return statInternal(path, followSymlinks); } @@ -217,7 +216,7 @@ protected UnixFileStatus statInternal(PathFragment path, boolean followSymlinks) // catch and don't re-throw. @Override @Nullable - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { String name = path.getPathString(); long startTime = Profiler.nanoTimeMaybe(); var comp = Blocker.begin(); @@ -232,7 +231,7 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { } @Override - protected boolean exists(PathFragment path, boolean followSymlinks) { + public boolean exists(PathFragment path, boolean followSymlinks) { return statNullable(path, followSymlinks) != null; } @@ -242,7 +241,7 @@ protected boolean exists(PathFragment path, boolean followSymlinks) { */ @Override @Nullable - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { String name = path.getPathString(); long startTime = Profiler.nanoTimeMaybe(); var comp = Blocker.begin(); @@ -269,17 +268,17 @@ protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) thro } @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { return (statInternal(path, true).getPermissions() & 0400) != 0; } @Override - protected boolean isWritable(PathFragment path) throws IOException { + public boolean isWritable(PathFragment path) throws IOException { return (statInternal(path, true).getPermissions() & 0200) != 0; } @Override - protected boolean isExecutable(PathFragment path) throws IOException { + public boolean isExecutable(PathFragment path) throws IOException { return (statInternal(path, true).getPermissions() & 0100) != 0; } @@ -303,7 +302,7 @@ private void modifyPermissionBits(PathFragment path, int permissionBits, boolean } @Override - protected void setReadable(PathFragment path, boolean readable) throws IOException { + public void setReadable(PathFragment path, boolean readable) throws IOException { modifyPermissionBits(path, 0400, readable); } @@ -313,12 +312,12 @@ public void setWritable(PathFragment path, boolean writable) throws IOException } @Override - protected void setExecutable(PathFragment path, boolean executable) throws IOException { + public void setExecutable(PathFragment path, boolean executable) throws IOException { modifyPermissionBits(path, 0111, executable); } @Override - protected void chmod(PathFragment path, int mode) throws IOException { + public void chmod(PathFragment path, int mode) throws IOException { var comp = Blocker.begin(); try { NativePosixFiles.chmod(path.toString(), mode); @@ -370,7 +369,7 @@ public boolean createDirectory(PathFragment path) throws IOException { } @Override - protected boolean createWritableDirectory(PathFragment path) throws IOException { + public boolean createWritableDirectory(PathFragment path) throws IOException { var comp = Blocker.begin(); try { return NativePosixFiles.mkdirWritable(path.toString()); @@ -391,7 +390,7 @@ public void createDirectoryAndParents(PathFragment path) throws IOException { } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { var comp = Blocker.begin(); @@ -403,7 +402,7 @@ protected void createSymbolicLink( } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { // Note that the default implementation of readSymbolicLinkUnchecked calls this method and thus // is optimal since we only make one system call in here. String name = path.toString(); @@ -430,12 +429,12 @@ public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IO } @Override - protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { return stat(path, followSymlinks).getSize(); } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { String name = path.toString(); long startTime = Profiler.nanoTimeMaybe(); var comp = Blocker.begin(); @@ -448,7 +447,7 @@ protected boolean delete(PathFragment path) throws IOException { } @Override - protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { return stat(path, followSymlinks).getLastModifiedTime(); } @@ -485,14 +484,14 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) @Override @Nullable - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { // Attempt to obtain the digest from an extended attribute attached to the file. This is much // faster than reading and digesting the file's contents on the fly, especially for large files. return hashAttributeName.isEmpty() ? null : getxattr(path, hashAttributeName, true); } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { String name = path.toString(); long startTime = Profiler.nanoTimeMaybe(); try { @@ -503,7 +502,7 @@ protected byte[] getDigest(PathFragment path) throws IOException { } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { var comp = Blocker.begin(); try { @@ -514,7 +513,7 @@ protected void createFSDependentHardLink(PathFragment linkPath, PathFragment ori } @Override - protected void deleteTreesBelow(PathFragment dir) throws IOException { + public void deleteTreesBelow(PathFragment dir) throws IOException { if (isDirectory(dir, /*followSymlinks=*/ false)) { long startTime = Profiler.nanoTimeMaybe(); var comp = Blocker.begin(); @@ -528,12 +527,12 @@ protected void deleteTreesBelow(PathFragment dir) throws IOException { } @Override - protected File getIoFile(PathFragment path) { + public File getIoFile(PathFragment path) { return new File(StringEncoding.internalToPlatform(path.getPathString())); } @Override - protected java.nio.file.Path getNioPath(PathFragment path) { + public java.nio.file.Path getNioPath(PathFragment path) { return java.nio.file.Path.of(StringEncoding.internalToPlatform(path.getPathString())); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java index dd5fae93254c45..78f31c9640be1b 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java @@ -48,7 +48,7 @@ public AbstractFileSystem(DigestHashFunction digestFunction) { } @Override - protected InputStream getInputStream(PathFragment path) throws IOException { + public InputStream getInputStream(PathFragment path) throws IOException { // This loop is a workaround for an apparent bug in FileInputStream.open, which delegates // ultimately to JVM_Open in the Hotspot JVM. This call is not EINTR-safe, so we must do the // retry here. @@ -99,7 +99,7 @@ private InputStream createMaybeProfiledInputStream(PathFragment path) throws IOE Sets.immutableEnumSet(READ, WRITE, CREATE, TRUNCATE_EXISTING); @Override - protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { + public SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { boolean shouldProfile = profiler.isActive() && profiler.isProfiling(ProfilerTask.VFS_OPEN); long startTime = Profiler.nanoTimeMaybe(); @@ -136,7 +136,7 @@ protected OutputStream createFileOutputStream(PathFragment path, boolean append, } @Override - protected OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) throws IOException { try { return createFileOutputStream(path, append, internal); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystemWithCustomStat.java b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystemWithCustomStat.java index 26f824e9a30b0a..79b884dc64d69e 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystemWithCustomStat.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystemWithCustomStat.java @@ -26,30 +26,30 @@ public AbstractFileSystemWithCustomStat(DigestHashFunction hashFunction) { } @Override - protected boolean isFile(PathFragment path, boolean followSymlinks) { + public boolean isFile(PathFragment path, boolean followSymlinks) { FileStatus stat = statNullable(path, followSymlinks); return stat != null ? stat.isFile() : false; } @Override - protected boolean isSpecialFile(PathFragment path, boolean followSymlinks) { + public boolean isSpecialFile(PathFragment path, boolean followSymlinks) { FileStatus stat = statNullable(path, followSymlinks); return stat != null ? stat.isSpecialFile() : false; } @Override - protected boolean isSymbolicLink(PathFragment path) { + public boolean isSymbolicLink(PathFragment path) { FileStatus stat = statNullable(path, false); return stat != null ? stat.isSymbolicLink() : false; } @Override - protected boolean isDirectory(PathFragment path, boolean followSymlinks) { + public boolean isDirectory(PathFragment path, boolean followSymlinks) { FileStatus stat = statNullable(path, followSymlinks); return stat != null ? stat.isDirectory() : false; } @Override - protected abstract FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException; + public abstract FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException; } 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 3b40773918b8b6..80969d0409aaaa 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 @@ -204,7 +204,7 @@ public String getFileSystemType(PathFragment path) { *

This method is not atomic -- concurrent modifications for the same path will result in * undefined behavior. */ - protected boolean createWritableDirectory(PathFragment path) throws IOException { + public boolean createWritableDirectory(PathFragment path) throws IOException { FileStatus stat = statNullable(path, /* followSymlinks= */ false); if (stat == null) { return createDirectory(path); @@ -232,10 +232,10 @@ protected boolean createWritableDirectory(PathFragment path) throws IOException * returns false, this method will throw an {@link UnsupportedOperationException} if {@code * followSymLinks=false}. */ - protected abstract long getFileSize(PathFragment path, boolean followSymlinks) throws IOException; + public abstract long getFileSize(PathFragment path, boolean followSymlinks) throws IOException; /** Deletes the file denoted by {@code path}. See {@link Path#delete} for specification. */ - protected abstract boolean delete(PathFragment path) throws IOException; + public abstract boolean delete(PathFragment path) throws IOException; /** * Deletes all directory trees recursively beneath the given path and removes that path as well. @@ -243,7 +243,7 @@ protected boolean createWritableDirectory(PathFragment path) throws IOException * @param path the directory hierarchy to remove * @throws IOException if the hierarchy cannot be removed successfully */ - protected void deleteTree(PathFragment path) throws IOException { + public void deleteTree(PathFragment path) throws IOException { deleteTreesBelow(path); delete(path); } @@ -261,7 +261,7 @@ protected void deleteTree(PathFragment path) throws IOException { * @param dir the directory hierarchy to remove * @throws IOException if the hierarchy cannot be removed successfully */ - protected void deleteTreesBelow(PathFragment dir) throws IOException { + public void deleteTreesBelow(PathFragment dir) throws IOException { if (isDirectory(dir, /* followSymlinks= */ false)) { Collection entries; try { @@ -315,7 +315,7 @@ protected void deleteTreesBelow(PathFragment dir) throws IOException { * returns false, this method will throw an {@link UnsupportedOperationException} if {@code * followSymLinks=false}. */ - protected abstract long getLastModifiedTime(PathFragment path, boolean followSymlinks) + public abstract long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException; /** @@ -351,7 +351,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) * file. */ @Nullable - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { return null; } @@ -363,7 +363,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException { * @return a new byte array containing the file's digest * @throws IOException if the digest could not be computed for any reason */ - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { return new ByteSource() { @Override public InputStream openStream() throws IOException { @@ -376,7 +376,7 @@ public InputStream openStream() throws IOException { * Returns true if "path" denotes an existing symbolic link. See {@link Path#isSymbolicLink} for * specification. */ - protected abstract boolean isSymbolicLink(PathFragment path); + public abstract boolean isSymbolicLink(PathFragment path); /** * Appends a single regular path segment 'child' to 'dir', recursively resolving symbolic links in @@ -435,7 +435,7 @@ protected final PathFragment appendSegment(PathFragment dir, String child, int m * @throws IOException if the file did not exist, or a parent directory could not be searched */ @Nullable - protected PathFragment resolveOneLink(PathFragment path) throws IOException { + public PathFragment resolveOneLink(PathFragment path) throws IOException { try { return readSymbolicLink(path); } catch (NotASymlinkException e) { @@ -458,7 +458,7 @@ protected PathFragment resolveOneLink(PathFragment path) throws IOException { * Returns the canonical path for the given path, which must be absolute. See {@link * Path#resolveSymbolicLinks} for specification. */ - protected Path resolveSymbolicLinks(PathFragment path) throws IOException { + public Path resolveSymbolicLinks(PathFragment path) throws IOException { checkArgument(path.isAbsolute()); PathFragment parentNode = path.getParentDirectory(); return parentNode == null @@ -476,7 +476,7 @@ protected Path resolveSymbolicLinks(PathFragment path) throws IOException { * we still try to follow Unix-like semantics of failing fast in case of non-existent files (or in * case of permission issues). */ - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { FileStatus status = new FileStatus() { volatile Boolean isFile; volatile Boolean isDirectory; @@ -543,7 +543,7 @@ public long getNodeId() { /** Like stat(), but returns null on failures instead of throwing. */ @Nullable - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { try { return stat(path, followSymlinks); } catch (IOException e) { @@ -558,7 +558,7 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { * instantiated filesystem can catch such errors, it should override this method to do so. */ @Nullable - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { try { return stat(path, followSymlinks); } catch (FileNotFoundException e) { @@ -570,19 +570,19 @@ protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) thro * Returns true iff {@code path} denotes an existing directory. See {@link * Path#isDirectory(Symlinks)} for specification. */ - protected abstract boolean isDirectory(PathFragment path, boolean followSymlinks); + public abstract boolean isDirectory(PathFragment path, boolean followSymlinks); /** * Returns true iff {@code path} denotes an existing regular or special file. See {@link * Path#isFile(Symlinks)} for specification. */ - protected abstract boolean isFile(PathFragment path, boolean followSymlinks); + public abstract boolean isFile(PathFragment path, boolean followSymlinks); /** * Returns true iff {@code path} denotes a special file. See {@link Path#isSpecialFile(Symlinks)} * for specification. */ - protected abstract boolean isSpecialFile(PathFragment path, boolean followSymlinks); + public abstract boolean isSpecialFile(PathFragment path, boolean followSymlinks); /** * Creates a symbolic link. See {@link Path#createSymbolicLink(Path, SymlinkTargetType)} for @@ -591,7 +591,7 @@ protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) thro *

Note: for {@link FileSystem}s where {@link #supportsSymbolicLinksNatively(PathFragment)} * returns false, this method will throw an {@link UnsupportedOperationException} */ - protected abstract void createSymbolicLink( + public abstract void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType hint) throws IOException; @@ -601,7 +601,7 @@ protected abstract void createSymbolicLink( *

Note: for {@link FileSystem}s where {@link #supportsSymbolicLinksNatively(PathFragment)} * returns false, this method will throw an {@link UnsupportedOperationException} */ - protected final void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) + public final void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) throws IOException { createSymbolicLink(linkPath, targetFragment, SymlinkTargetType.UNSPECIFIED); } @@ -616,7 +616,7 @@ protected final void createSymbolicLink(PathFragment linkPath, PathFragment targ * @throws NotASymlinkException if the current path is not a symbolic link * @throws IOException if the contents of the link could not be read for any reason. */ - protected abstract PathFragment readSymbolicLink(PathFragment path) throws IOException; + public abstract PathFragment readSymbolicLink(PathFragment path) throws IOException; /** * Returns the target of a symbolic link, under the assumption that the given path is indeed a @@ -625,7 +625,7 @@ protected final void createSymbolicLink(PathFragment linkPath, PathFragment targ * * @throws IOException if the contents of the link could not be read for any reason. */ - protected PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { + public PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { return readSymbolicLink(path); } @@ -638,7 +638,7 @@ public boolean exists(PathFragment path) { * Returns true iff {@code path} denotes an existing file of any kind. See {@link * Path#exists(Symlinks)} for specification. */ - protected abstract boolean exists(PathFragment path, boolean followSymlinks); + public abstract boolean exists(PathFragment path, boolean followSymlinks); /** * Returns a collection containing the names of all entities within the directory denoted by the @@ -646,7 +646,7 @@ public boolean exists(PathFragment path) { * * @throws IOException if there was an error reading the directory entries */ - protected abstract Collection getDirectoryEntries(PathFragment path) throws IOException; + public abstract Collection getDirectoryEntries(PathFragment path) throws IOException; protected static Dirent.Type direntFromStat(@Nullable FileStatus stat) { if (stat == null) { @@ -673,8 +673,7 @@ protected static Dirent.Type direntFromStat(@Nullable FileStatus stat) { * resolving the directory whose entries are to be read. * @throws IOException if there was an error reading the directory entries */ - protected Collection readdir(PathFragment path, boolean followSymlinks) - throws IOException { + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { Collection children = getDirectoryEntries(path); List dirents = Lists.newArrayListWithCapacity(children.size()); for (String child : children) { @@ -690,7 +689,7 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) * * @throws IOException if there was an error reading the file's metadata */ - protected abstract boolean isReadable(PathFragment path) throws IOException; + public abstract boolean isReadable(PathFragment path) throws IOException; /** * Sets the file to readable (if the argument is true) or non-readable (if the argument is false) @@ -701,14 +700,14 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) * * @throws IOException if there was an error reading or writing the file's metadata */ - protected abstract void setReadable(PathFragment path, boolean readable) throws IOException; + public abstract void setReadable(PathFragment path, boolean readable) throws IOException; /** * Returns true iff the file represented by {@code path} is writable. * * @throws IOException if there was an error reading the file's metadata */ - protected abstract boolean isWritable(PathFragment path) throws IOException; + public abstract boolean isWritable(PathFragment path) throws IOException; /** * Sets the file to writable (if the argument is true) or non-writable (if the argument is false) @@ -725,7 +724,7 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) * * @throws IOException if there was an error reading the file's metadata */ - protected abstract boolean isExecutable(PathFragment path) throws IOException; + public abstract boolean isExecutable(PathFragment path) throws IOException; /** * Sets the file to executable, if the argument is true. It is currently not supported to unset @@ -737,7 +736,7 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) * * @throws IOException if there was an error reading or writing the file's metadata */ - protected abstract void setExecutable(PathFragment path, boolean executable) throws IOException; + public abstract void setExecutable(PathFragment path, boolean executable) throws IOException; /** * Sets the file permissions. If permission changes on this {@link FileSystem} are slow (e.g. one @@ -750,7 +749,7 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) * * @throws IOException if there was an error reading or writing the file's metadata */ - protected void chmod(PathFragment path, int mode) throws IOException { + public void chmod(PathFragment path, int mode) throws IOException { setReadable(path, (mode & 0400) != 0); setWritable(path, (mode & 0200) != 0); setExecutable(path, (mode & 0100) != 0); @@ -762,14 +761,14 @@ protected void chmod(PathFragment path, int mode) throws IOException { * @throws FileNotFoundException if the file does not exist * @throws IOException if there was an error opening the file for reading */ - protected abstract InputStream getInputStream(PathFragment path) throws IOException; + public abstract InputStream getInputStream(PathFragment path) throws IOException; /** * Returns a {@link SeekableByteChannel} for writing to a file at provided path. * *

Truncates the target file, therefore it cannot be used to read already existing files. */ - protected abstract SeekableByteChannel createReadWriteByteChannel(PathFragment path) + public abstract SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException; /** @@ -799,8 +798,8 @@ protected final OutputStream getOutputStream(PathFragment path, boolean append) * @param internal whether the file is a Bazel internal file * @throws IOException if there was an error opening the file for writing */ - protected abstract OutputStream getOutputStream( - PathFragment path, boolean append, boolean internal) throws IOException; + public abstract OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + throws IOException; /** * Renames the file denoted by "sourceNode" to the location "targetNode". See {@link @@ -818,8 +817,7 @@ public abstract void renameTo(PathFragment sourcePath, PathFragment targetPath) * @param originalPath The path of the original file * @throws IOException if the original file does not exist or the link file already exists */ - protected void createHardLink(PathFragment linkPath, PathFragment originalPath) - throws IOException { + public void createHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { if (!exists(originalPath)) { throw new FileNotFoundException( @@ -845,21 +843,21 @@ protected void createHardLink(PathFragment linkPath, PathFragment originalPath) * @param originalPath The path of the original file * @throws IOException if there was an I/O error */ - protected abstract void createFSDependentHardLink( - PathFragment linkPath, PathFragment originalPath) throws IOException; + public abstract void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + throws IOException; /** * Prefetch all directories and symlinks within the package rooted at "path". Enter at most * "maxDirs" total directories. Specializations for high-latency remote filesystems may wish to * implement this in order to warm the filesystem's internal caches. */ - protected void prefetchPackageAsync(PathFragment path, int maxDirs) {} + public void prefetchPackageAsync(PathFragment path, int maxDirs) {} /** * Returns a {@link File} object for the given path. This method is only supported by file system * implementations that are backed by the local file system. */ - protected File getIoFile(PathFragment path) { + public File getIoFile(PathFragment path) { throw new UnsupportedOperationException( "getIoFile() not supported for " + getClass().getName()); } @@ -868,7 +866,7 @@ protected File getIoFile(PathFragment path) { * Returns a {@link java.nio.file.Path} object for the given path. This method is only supported * by file system implementations that are backed by the local file system. */ - protected java.nio.file.Path getNioPath(PathFragment path) { + public java.nio.file.Path getNioPath(PathFragment path) { throw new UnsupportedOperationException( "getNioPath() not supported for " + getClass().getName()); } @@ -877,8 +875,7 @@ protected java.nio.file.Path getNioPath(PathFragment path) { * Returns the path of a new temporary directory with the given prefix created under the given * parent path, but not necessarily with secure permissions. */ - protected PathFragment createTempDirectory(PathFragment parent, String prefix) - throws IOException { + public PathFragment createTempDirectory(PathFragment parent, String prefix) throws IOException { SecureRandom rand = new SecureRandom(); while (true) { PathFragment candidate = parent.getRelative(prefix + Long.toUnsignedString(rand.nextLong())); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java index b1ae100c4e7339..7d696fdde568f4 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java @@ -65,20 +65,20 @@ public JavaIoFileSystem(DigestHashFunction hashFunction) { } @Override - protected File getIoFile(PathFragment path) { + public File getIoFile(PathFragment path) { return new File(StringEncoding.internalToPlatform(path.getPathString())); } /** * Returns a {@link java.nio.file.Path} representing the same path as provided {@code path}. * - *

Note: while it's possible to use {@link #getIoFile(PathFragment)} in combination with {@link - * File#toPath()} to achieve essentially the same, using this method is preferable because it - * avoids extra allocations and does not lose track of the underlying Java filesystem, which is - * useful for some in-memory filesystem implementations like JimFS. + *

Note: while it's possible to use {@link FileSystem#getIoFile(PathFragment)} in combination + * with {@link File#toPath()} to achieve essentially the same, using this method is preferable + * because it avoids extra allocations and does not lose track of the underlying Java filesystem, + * which is useful for some in-memory filesystem implementations like JimFS. */ @Override - protected java.nio.file.Path getNioPath(PathFragment path) { + public java.nio.file.Path getNioPath(PathFragment path) { return Paths.get(StringEncoding.internalToPlatform(path.getPathString())); } @@ -87,7 +87,7 @@ private LinkOption[] linkOpts(boolean followSymlinks) { } @Override - protected Collection getDirectoryEntries(PathFragment path) throws IOException { + public Collection getDirectoryEntries(PathFragment path) throws IOException { File file = getIoFile(path); String[] entries; long startTime = Profiler.nanoTimeMaybe(); @@ -107,7 +107,7 @@ protected Collection getDirectoryEntries(PathFragment path) throws IOExc } @Override - protected boolean exists(PathFragment path, boolean followSymlinks) { + public boolean exists(PathFragment path, boolean followSymlinks) { long startTime = Profiler.nanoTimeMaybe(); try { java.nio.file.Path nioPath = getNioPath(path); @@ -120,7 +120,7 @@ protected boolean exists(PathFragment path, boolean followSymlinks) { } @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { File file = getIoFile(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -134,7 +134,7 @@ protected boolean isReadable(PathFragment path) throws IOException { } @Override - protected boolean isWritable(PathFragment path) throws IOException { + public boolean isWritable(PathFragment path) throws IOException { File file = getIoFile(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -152,7 +152,7 @@ protected boolean isWritable(PathFragment path) throws IOException { } @Override - protected boolean isExecutable(PathFragment path) throws IOException { + public boolean isExecutable(PathFragment path) throws IOException { File file = getIoFile(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -166,7 +166,7 @@ protected boolean isExecutable(PathFragment path) throws IOException { } @Override - protected void setReadable(PathFragment path, boolean readable) throws IOException { + public void setReadable(PathFragment path, boolean readable) throws IOException { File file = getIoFile(path); if (!file.exists()) { throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR); @@ -188,7 +188,7 @@ public void setWritable(PathFragment path, boolean writable) throws IOException } @Override - protected void setExecutable(PathFragment path, boolean executable) throws IOException { + public void setExecutable(PathFragment path, boolean executable) throws IOException { File file = getIoFile(path); if (!file.exists()) { throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR); @@ -279,7 +279,7 @@ private boolean linkExists(File file) { } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { java.nio.file.Path nioPath = getNioPath(linkPath); @@ -298,7 +298,7 @@ protected void createSymbolicLink( } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { java.nio.file.Path nioPath = getNioPath(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -347,7 +347,7 @@ public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IO } @Override - protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { long startTime = Profiler.nanoTimeMaybe(); try { return stat(path, followSymlinks).getSize(); @@ -357,7 +357,7 @@ protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOE } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { java.nio.file.Path nioPath = getNioPath(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -398,7 +398,7 @@ protected boolean delete(PathFragment path) throws IOException { } @Override - protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { File file = getIoFile(path); long startTime = Profiler.nanoTimeMaybe(); try { @@ -428,7 +428,7 @@ public void setLastModifiedTime(PathFragment path, long newTime) throws IOExcept } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { String name = path.toString(); long startTime = Profiler.nanoTimeMaybe(); try { @@ -447,7 +447,7 @@ protected byte[] getDigest(PathFragment path) throws IOException { * case of permission issues). */ @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { java.nio.file.Path nioPath = getNioPath(path); final BasicFileAttributes attributes; try { @@ -506,7 +506,7 @@ public long getNodeId() { @Override @Nullable - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) { try { return stat(path, followSymlinks); } catch (FileNotFoundException e) { @@ -523,7 +523,7 @@ protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) { } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { Files.createLink(getNioPath(linkPath), getNioPath(originalPath)); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java index 1bd9886787ea7d..8a592bf7117a04 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java @@ -69,7 +69,7 @@ public boolean createDirectory(PathFragment path) throws IOException { } @Override - protected boolean createWritableDirectory(PathFragment path) throws IOException { + public boolean createWritableDirectory(PathFragment path) throws IOException { return delegateFs.createWritableDirectory(toDelegatePath(path)); } @@ -79,17 +79,17 @@ public void createDirectoryAndParents(PathFragment path) throws IOException { } @Override - protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { return delegateFs.getFileSize(toDelegatePath(path), followSymlinks); } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { return delegateFs.delete(toDelegatePath(path)); } @Override - protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { return delegateFs.getLastModifiedTime(toDelegatePath(path), followSymlinks); } @@ -99,39 +99,39 @@ public void setLastModifiedTime(PathFragment path, long newTime) throws IOExcept } @Override - protected boolean isSymbolicLink(PathFragment path) { + public boolean isSymbolicLink(PathFragment path) { return delegateFs.isSymbolicLink(toDelegatePath(path)); } @Override - protected boolean isDirectory(PathFragment path, boolean followSymlinks) { + public boolean isDirectory(PathFragment path, boolean followSymlinks) { return delegateFs.isDirectory(toDelegatePath(path), followSymlinks); } @Override - protected boolean isFile(PathFragment path, boolean followSymlinks) { + public boolean isFile(PathFragment path, boolean followSymlinks) { return delegateFs.isFile(toDelegatePath(path), followSymlinks); } @Override - protected boolean isSpecialFile(PathFragment path, boolean followSymlinks) { + public boolean isSpecialFile(PathFragment path, boolean followSymlinks) { return delegateFs.isSpecialFile(toDelegatePath(path), followSymlinks); } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { delegateFs.createSymbolicLink(toDelegatePath(linkPath), targetFragment, type); } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { return fromDelegatePath(delegateFs.readSymbolicLink(toDelegatePath(path))); } @Override - protected boolean exists(PathFragment path, boolean followSymlinks) { + public boolean exists(PathFragment path, boolean followSymlinks) { return delegateFs.exists(toDelegatePath(path), followSymlinks); } @@ -141,22 +141,22 @@ public boolean exists(PathFragment path) { } @Override - protected Collection getDirectoryEntries(PathFragment path) throws IOException { + public Collection getDirectoryEntries(PathFragment path) throws IOException { return delegateFs.getDirectoryEntries(toDelegatePath(path)); } @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { return delegateFs.isReadable(toDelegatePath(path)); } @Override - protected void setReadable(PathFragment path, boolean readable) throws IOException { + public void setReadable(PathFragment path, boolean readable) throws IOException { delegateFs.setReadable(toDelegatePath(path), readable); } @Override - protected boolean isWritable(PathFragment path) throws IOException { + public boolean isWritable(PathFragment path) throws IOException { return delegateFs.isWritable(toDelegatePath(path)); } @@ -166,27 +166,27 @@ public void setWritable(PathFragment path, boolean writable) throws IOException } @Override - protected boolean isExecutable(PathFragment path) throws IOException { + public boolean isExecutable(PathFragment path) throws IOException { return delegateFs.isExecutable(toDelegatePath(path)); } @Override - protected void setExecutable(PathFragment path, boolean executable) throws IOException { + public void setExecutable(PathFragment path, boolean executable) throws IOException { delegateFs.setExecutable(toDelegatePath(path), executable); } @Override - protected InputStream getInputStream(PathFragment path) throws IOException { + public InputStream getInputStream(PathFragment path) throws IOException { return delegateFs.getInputStream(toDelegatePath(path)); } @Override - protected SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { + public SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { return delegateFs.createReadWriteByteChannel(toDelegatePath(path)); } @Override - protected OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) throws IOException { return delegateFs.getOutputStream(toDelegatePath(path), append, internal); } @@ -197,7 +197,7 @@ public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IO } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { delegateFs.createFSDependentHardLink(toDelegatePath(linkPath), toDelegatePath(originalPath)); } @@ -208,12 +208,12 @@ public String getFileSystemType(PathFragment path) { } @Override - protected void deleteTree(PathFragment path) throws IOException { + public void deleteTree(PathFragment path) throws IOException { delegateFs.deleteTree(toDelegatePath(path)); } @Override - protected void deleteTreesBelow(PathFragment dir) throws IOException { + public void deleteTreesBelow(PathFragment dir) throws IOException { delegateFs.deleteTreesBelow(toDelegatePath(dir)); } @@ -224,81 +224,78 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) } @Override - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { return delegateFs.getFastDigest(toDelegatePath(path)); } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { return delegateFs.getDigest(toDelegatePath(path)); } @Override - protected PathFragment resolveOneLink(PathFragment path) throws IOException { + public PathFragment resolveOneLink(PathFragment path) throws IOException { return delegateFs.resolveOneLink(toDelegatePath(path)); } @Override - protected Path resolveSymbolicLinks(PathFragment path) throws IOException { + public Path resolveSymbolicLinks(PathFragment path) throws IOException { return getPath( fromDelegatePath(delegateFs.resolveSymbolicLinks(toDelegatePath(path)).asFragment())); } @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { return delegateFs.stat(toDelegatePath(path), followSymlinks); } @Override - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { return delegateFs.statNullable(toDelegatePath(path), followSymlinks); } @Override - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { return delegateFs.statIfFound(toDelegatePath(path), followSymlinks); } @Override - protected PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { + public PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { return delegateFs.readSymbolicLinkUnchecked(toDelegatePath(path)); } @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) - throws IOException { + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { return delegateFs.readdir(toDelegatePath(path), followSymlinks); } @Override - protected void chmod(PathFragment path, int mode) throws IOException { + public void chmod(PathFragment path, int mode) throws IOException { delegateFs.chmod(toDelegatePath(path), mode); } @Override - protected void createHardLink(PathFragment linkPath, PathFragment originalPath) - throws IOException { + public void createHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { delegateFs.createHardLink(toDelegatePath(linkPath), toDelegatePath(originalPath)); } @Override - protected void prefetchPackageAsync(PathFragment path, int maxDirs) { + public void prefetchPackageAsync(PathFragment path, int maxDirs) { delegateFs.prefetchPackageAsync(toDelegatePath(path), maxDirs); } @Override - protected File getIoFile(PathFragment path) { + public File getIoFile(PathFragment path) { return delegateFs.getIoFile(toDelegatePath(path)); } @Override - protected java.nio.file.Path getNioPath(PathFragment path) { + public java.nio.file.Path getNioPath(PathFragment path) { return delegateFs.getNioPath(toDelegatePath(path)); } @Override - protected PathFragment createTempDirectory(PathFragment parent, String prefix) - throws IOException { + public PathFragment createTempDirectory(PathFragment parent, String prefix) throws IOException { return delegateFs.createTempDirectory(toDelegatePath(parent), prefix); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java index eb87ac7dd46715..a1a393d5d483f6 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/ReadonlyFileSystemWithCustomStat.java @@ -30,13 +30,13 @@ protected IOException modificationException() { } @Override - protected OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) throws IOException { throw modificationException(); } @Override - protected void setReadable(PathFragment path, boolean readable) throws IOException { + public void setReadable(PathFragment path, boolean readable) throws IOException { throw modificationException(); } @@ -46,7 +46,7 @@ public void setWritable(PathFragment path, boolean writable) throws IOException } @Override - protected void setExecutable(PathFragment path, boolean executable) { + public void setExecutable(PathFragment path, boolean executable) { throw new UnsupportedOperationException("setExecutable"); } @@ -81,14 +81,14 @@ public void createDirectoryAndParents(PathFragment path) throws IOException { } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { throw modificationException(); } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { throw modificationException(); } @@ -99,7 +99,7 @@ public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IO } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { throw modificationException(); } @@ -108,4 +108,3 @@ public void setLastModifiedTime(PathFragment path, long newTime) throws IOExcept throw modificationException(); } } - diff --git a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java index 287723b00b85a4..993d128f98514e 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystem.java @@ -376,7 +376,7 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws @Override @Nullable - protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { return inodeStatErrno(path, followSymlinks).inode(); } @@ -409,7 +409,7 @@ private InMemoryContentInfo inodeStat(PathFragment path, boolean followSymlinks) */ @Nullable @Override - protected PathFragment resolveOneLink(PathFragment path) throws IOException { + public PathFragment resolveOneLink(PathFragment path) throws IOException { // Beware, this seemingly simple code belies the complex specification of // FileSystem.resolveOneLink(). InMemoryContentInfo status = inodeStat(path, false); @@ -417,24 +417,24 @@ protected PathFragment resolveOneLink(PathFragment path) throws IOException { } @Override - protected boolean exists(PathFragment path, boolean followSymlinks) { + public boolean exists(PathFragment path, boolean followSymlinks) { return statNullable(path, followSymlinks) != null; } @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { InMemoryContentInfo status = inodeStat(path, true); return status.isReadable(); } @Override - protected synchronized void setReadable(PathFragment path, boolean readable) throws IOException { + public synchronized void setReadable(PathFragment path, boolean readable) throws IOException { InMemoryContentInfo status = inodeStat(path, true); status.setReadable(readable); } @Override - protected boolean isWritable(PathFragment path) throws IOException { + public boolean isWritable(PathFragment path) throws IOException { InMemoryContentInfo status = inodeStat(path, true); return status.isWritable(); } @@ -446,14 +446,13 @@ public synchronized void setWritable(PathFragment path, boolean writable) throws } @Override - protected boolean isExecutable(PathFragment path) throws IOException { + public boolean isExecutable(PathFragment path) throws IOException { InMemoryContentInfo status = inodeStat(path, true); return status.isExecutable(); } @Override - protected synchronized void setExecutable(PathFragment path, boolean executable) - throws IOException { + public synchronized void setExecutable(PathFragment path, boolean executable) throws IOException { InMemoryContentInfo status = inodeStat(path, true); status.setExecutable(executable); } @@ -522,7 +521,7 @@ public void createDirectoryAndParents(PathFragment path) throws IOException { } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment path, PathFragment targetFragment, SymlinkTargetType type) throws IOException { if (isRootDirectory(path)) { throw Errno.EACCES.exception(path); @@ -539,7 +538,7 @@ protected void createSymbolicLink( } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { InMemoryContentInfo status = inodeStat(path, false); if (status.isSymbolicLink()) { Preconditions.checkState(status instanceof InMemoryLinkInfo, status); @@ -549,13 +548,12 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { } @Override - protected long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { return stat(path, followSymlinks).getSize(); } @Override - protected synchronized Collection getDirectoryEntries(PathFragment path) - throws IOException { + public synchronized Collection getDirectoryEntries(PathFragment path) throws IOException { InMemoryDirectoryInfo dirInfo = getDirectory(path); if (!dirInfo.isReadable()) { throw Errno.EACCES.exception(path); @@ -572,7 +570,7 @@ protected synchronized Collection getDirectoryEntries(PathFragment path) } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { if (isRootDirectory(path)) { throw Errno.EBUSY.exception(path); } @@ -592,7 +590,7 @@ protected boolean delete(PathFragment path) throws IOException { } @Override - protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { return stat(path, followSymlinks).getLastModifiedTime(); } @@ -604,19 +602,19 @@ public synchronized void setLastModifiedTime(PathFragment path, long newTime) th } @Override - protected synchronized InputStream getInputStream(PathFragment path) throws IOException { + public synchronized InputStream getInputStream(PathFragment path) throws IOException { return statFile(path).getInputStream(); } @Override - protected synchronized SeekableByteChannel createReadWriteByteChannel(PathFragment path) + public synchronized SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { InMemoryContentInfo status = getOrCreateWritableInode(path); return ((FileInfo) status).createReadWriteByteChannel(); } @Override - protected synchronized byte[] getFastDigest(PathFragment path) throws IOException { + public synchronized byte[] getFastDigest(PathFragment path) throws IOException { return statFile(path).getFastDigest(); } @@ -668,7 +666,7 @@ protected InMemoryContentInfo getOrCreateWritableInode(PathFragment path) throws } @Override - protected synchronized OutputStream getOutputStream( + public synchronized OutputStream getOutputStream( PathFragment path, boolean append, boolean internal) throws IOException { InMemoryContentInfo status = getOrCreateWritableInode(path); return ((FileInfo) status).getOutputStream(append); @@ -723,7 +721,7 @@ public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IO } @Override - protected void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) throws IOException { // Same check used when creating a symbolic link diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java index c663405062cd59..3f0ea5a713edf3 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java @@ -55,7 +55,7 @@ public String getFileSystemType(PathFragment path) { } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { long startTime = Profiler.nanoTimeMaybe(); try { return WindowsFileOperations.deletePath( @@ -70,13 +70,13 @@ protected boolean delete(PathFragment path) throws IOException { } @Override - protected boolean createWritableDirectory(PathFragment path) throws IOException { + public boolean createWritableDirectory(PathFragment path) throws IOException { // All directories are writable on Windows. return createDirectory(path); } @Override - protected void createSymbolicLink( + public void createSymbolicLink( PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type) throws IOException { PathFragment targetPath = @@ -114,7 +114,7 @@ protected void createSymbolicLink( } @Override - protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + public PathFragment readSymbolicLink(PathFragment path) throws IOException { java.nio.file.Path nioPath = getNioPath(path); return PathFragment.create( StringEncoding.platformToInternal( @@ -148,7 +148,7 @@ public static LinkOption[] symlinkOpts(boolean followSymlinks) { } @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { File file = getIoFile(path); final DosFileAttributes attributes; try { @@ -217,7 +217,7 @@ public int getPermissions() { } @Override - protected boolean isDirectory(PathFragment path, boolean followSymlinks) { + public boolean isDirectory(PathFragment path, boolean followSymlinks) { if (!followSymlinks) { try { if (isSymlinkOrJunction(getIoFile(path))) { @@ -231,13 +231,13 @@ protected boolean isDirectory(PathFragment path, boolean followSymlinks) { } @Override - protected void setReadable(PathFragment path, boolean readable) { + public void setReadable(PathFragment path, boolean readable) { // Windows does not have a notion of readable files. // https://github.com/openjdk/jdk/blob/e52a2aeeacaeb26c801b6e31f8e67e61b1ea2de3/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L473-L476 } @Override - protected void setExecutable(PathFragment path, boolean executable) { + public void setExecutable(PathFragment path, boolean executable) { // Windows does not have a notion of executable files. // https://github.com/openjdk/jdk/blob/e52a2aeeacaeb26c801b6e31f8e67e61b1ea2de3/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L473-L476 } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/InterruptedExceptionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/InterruptedExceptionTest.java index c88f4e44eb40d1..65e280bd836e58 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/InterruptedExceptionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/InterruptedExceptionTest.java @@ -40,7 +40,7 @@ public class InterruptedExceptionTest extends AnalysisTestCase { protected FileSystem createFileSystem() { return new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { if (path.toString().contains("causes_interrupt")) { mainThread.interrupt(); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java index 4faf1b270a2386..6b697f17f7a4b7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java @@ -136,7 +136,7 @@ void stubFastDigestError(Path path, IOException error) { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { if (stubbedFastDigestErrors.containsKey(path)) { throw stubbedFastDigestErrors.get(path); } diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ProgressReportingTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ProgressReportingTest.java index a0b12f86d797d1..d2a6b8cc91eee6 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/ProgressReportingTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/ProgressReportingTest.java @@ -53,7 +53,7 @@ protected ImmutableSet additionalEventsToCollect() { @Override protected FileSystem createFileSystem() { - return new UnixFileSystem(DigestHashFunction.SHA256, /*hashAttributeName=*/ "") { + return new UnixFileSystem(DigestHashFunction.SHA256, /* hashAttributeName= */ "") { private void recordAccess(PathOp op, PathFragment path) { if (receiver != null) { receiver.accept(path, op); @@ -61,7 +61,7 @@ private void recordAccess(PathOp op, PathFragment path) { } @Override - protected boolean delete(PathFragment path) throws IOException { + public boolean delete(PathFragment path) throws IOException { recordAccess(PathOp.DELETE, path); return super.delete(path); } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java index aa698adecb8066..8a348307c4167c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java @@ -53,7 +53,7 @@ public final void setUp() throws Exception { fs = new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - protected InputStream getInputStream(PathFragment path) throws IOException { + public InputStream getInputStream(PathFragment path) throws IOException { int c = calls.containsKey(path.toString()) ? calls.get(path.toString()) : 0; c++; calls.put(path.toString(), c); @@ -61,13 +61,13 @@ protected InputStream getInputStream(PathFragment path) throws IOException { } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { byte[] override = digestOverrides.get(path.getPathString()); return override != null ? override : super.getDigest(path); } @Override - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { return null; } }; diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java index 1f9fec8a2c7461..615080880441b0 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java @@ -171,12 +171,12 @@ protected static final class FakeActionFileSystem extends DelegateFileSystem { } @Override - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { return super.getDigest(path); } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { throw new UnsupportedOperationException(); } } diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java index 9ed5e7190c7c03..96bbed2a38891d 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java @@ -1997,7 +1997,7 @@ synchronized void throwExceptionOnGetInputStream(Path path, IOException exn) { } @Override - protected synchronized InputStream getInputStream(PathFragment path) throws IOException { + public synchronized InputStream getInputStream(PathFragment path) throws IOException { IOException exnToThrow = pathsToErrorOnGetInputStream.get(path); if (exnToThrow != null) { throw exnToThrow; diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java index f83e7b33524c2f..8ec87acb028a93 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java @@ -88,7 +88,7 @@ public FileStatus statNullable(PathFragment path, boolean followSymlinks) { } @Override - protected Collection readdir(PathFragment path, boolean followSymlinks) + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { Collection defaultResult = super.readdir(path, followSymlinks); return transformer.readdir(defaultResult, path, followSymlinks); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index b588f5594280ff..2536d6d4dd6a5c 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -144,7 +144,7 @@ public void sandboxInputMaterializeVirtualInput_parallelWritesForSameInput_write new InMemoryFileSystem(DigestHashFunction.SHA1) { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") // .await() inside - protected void setExecutable(PathFragment path, boolean executable) throws IOException { + public void setExecutable(PathFragment path, boolean executable) throws IOException { try { bothWroteTempFile.await(); finishProcessingSemaphore.acquire(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java index b64be275b9602c..768069c0a75963 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java @@ -183,7 +183,7 @@ protected class CustomInMemoryFs extends InMemoryFileSystem { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { return fastDigest ? getDigest(path) : null; } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index 2768dc6aa307ba..1dc9954f16cd8f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -1260,7 +1260,7 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws } @Override - protected synchronized InputStream getInputStream(PathFragment path) throws IOException { + public synchronized InputStream getInputStream(PathFragment path) throws IOException { if (badPathForRead != null && badPathForRead.asFragment().equals(path)) { throw new IOException("bad"); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index b8265c3c0d07a6..30025859cd8d39 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -212,7 +212,7 @@ public byte[] getDigest(PathFragment path) throws IOException { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { throw exception; } }; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 7efa8b5ebbe43f..3659f8033e3c7f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -564,7 +564,7 @@ public void testEmptyFile() throws Exception { new CustomInMemoryFs(manualClock) { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) { + public byte[] getFastDigest(PathFragment path) { return digest; } }); @@ -603,7 +603,7 @@ public void testUnreadableFileWithFastDigest() throws Exception { new CustomInMemoryFs(manualClock) { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) { + public byte[] getFastDigest(PathFragment path) { return path.getBaseName().equals("unreadable") ? expectedDigest : null; } }); @@ -904,7 +904,7 @@ public void testDigest() throws Exception { fs = new CustomInMemoryFs(manualClock) { @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { digestCalls.incrementAndGet(); return super.getDigest(path); } @@ -986,7 +986,7 @@ public void testFilesystemInconsistencies_getFastDigestAndIsReadableFailure() th createFsAndRoot( new CustomInMemoryFs(manualClock) { @Override - protected boolean isReadable(PathFragment path) throws IOException { + public boolean isReadable(PathFragment path) throws IOException { if (path.getBaseName().equals("unreadable")) { throw new IOException("isReadable failed"); } @@ -1828,7 +1828,7 @@ void stubFastDigestError(Path path, IOException error) { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") - protected byte[] getFastDigest(PathFragment path) throws IOException { + public byte[] getFastDigest(PathFragment path) throws IOException { if (stubbedFastDigestErrors.containsKey(path)) { throw stubbedFastDigestErrors.get(path); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java index 39e2fda5fc6f0c..495bd9d1ab0592 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java @@ -82,8 +82,7 @@ public Iterable> getCommandOptions(Command command) public FileSystem createFileSystem() throws Exception { return new DelegateFileSystem(super.createFileSystem()) { @Override - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) - throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { IOException e = throwOnNextStatIfFound.remove(path); if (e != null) { throw e; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index 2ec18bfe8da812..c3a7254c950a62 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java @@ -2159,7 +2159,7 @@ void throwExceptionOnGetInputStream(Path path, IOException exn) { } @Override - protected synchronized InputStream getInputStream(PathFragment path) throws IOException { + public synchronized InputStream getInputStream(PathFragment path) throws IOException { IOException exnToThrow = pathsToErrorOnGetInputStream.get(path); if (exnToThrow != null) { throw exnToThrow; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java index 40a8bb00e2acda..02e21bd4d2f505 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java @@ -62,8 +62,7 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes protected FileSystem createFileSystem() { return new DelegateFileSystem(super.createFileSystem()) { @Override - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) - throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { @Nullable FileStatus injectedStat = injectedStats.remove(path); if (injectedStat != null) { return injectedStat; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index ffef6cda86b2cf..9f48f323f469ab 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -128,8 +128,7 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe protected FileSystem createFileSystem() { return new DelegateFileSystem(super.createFileSystem()) { @Override - protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) - throws IOException { + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { if (pathsToPretendDontExist.contains(path)) { return null; } diff --git a/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java b/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java index 357a16ae9a680b..425ade54a15ca2 100644 --- a/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java +++ b/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java @@ -23,7 +23,7 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import org.junit.Test; -/** Test for {@link com.google.devtools.build.lib.unix.UnixFileSystem#getFastDigest}. */ +/** Test for {@link FileSystem#getFastDigest}. */ public class UnixDigestHashAttributeNameTest extends FileSystemTest { private static final byte[] FAKE_DIGEST = { 0x18, 0x5f, 0x3d, 0x33, 0x22, 0x71, 0x7e, 0x25, diff --git a/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java index 310177b44e7491..c753a881e8c689 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java @@ -40,13 +40,13 @@ public void testCache() throws Exception { FileSystem tracingFileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - protected byte[] getFastDigest(PathFragment path) { + public byte[] getFastDigest(PathFragment path) { getFastDigestCounter.incrementAndGet(); return null; } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path) throws IOException { getDigestCounter.incrementAndGet(); return super.getDigest(path); } @@ -80,12 +80,12 @@ public void manuallyComputeDigest() throws Exception { FileSystem noDigestFileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - protected byte[] getFastDigest(PathFragment path) { + public byte[] getFastDigest(PathFragment path) { throw new AssertionError("Unexpected call to getFastDigest"); } @Override - protected byte[] getDigest(PathFragment path) { + public byte[] getDigest(PathFragment path) { return digest; } }; From 48a7faebe531491a4962169cd1774c8d6b176710 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 3 Nov 2025 11:00:54 -0800 Subject: [PATCH 2/5] [8.7.0] Add a remote repo contents cache (cherry picked from commit b8589c3b278e3f5cee6ef85b0dcabb1cdcd69839) (cherry picked from commit f741d36a274376e5b4e7d85ccd95f7b62846f57c) --- src/main/cpp/blaze.cc | 5 + src/main/cpp/startup_options.cc | 7 +- src/main/cpp/startup_options.h | 4 + .../java/com/google/devtools/build/lib/BUILD | 12 + .../build/lib/actions/FileStateValue.java | 74 ++ .../com/google/devtools/build/lib/bazel/BUILD | 1 + .../lib/bazel/BazelRepositoryModule.java | 7 +- .../build/lib/bazel/repository/starlark/BUILD | 1 + .../starlark/StarlarkBaseExternalContext.java | 14 + .../remote/AbstractActionInputPrefetcher.java | 194 +++-- .../google/devtools/build/lib/remote/BUILD | 5 + .../remote/RemoteActionContextProvider.java | 4 + .../lib/remote/RemoteActionInputFetcher.java | 38 +- .../lib/remote/RemoteExecutionService.java | 24 +- .../RemoteExternalOverlayFileSystem.java | 677 ++++++++++++++++++ .../build/lib/remote/RemoteModule.java | 126 +++- .../remote/RemoteRepoContentsCacheImpl.java | 347 +++++++++ ...> RepositoryRemoteHelpersFactoryImpl.java} | 19 +- .../build/lib/remote/UploadManifest.java | 43 +- .../remote/options/RemoteStartupOptions.java | 40 ++ .../devtools/build/lib/remote/util/Utils.java | 16 +- .../com/google/devtools/build/lib/rules/BUILD | 1 + .../RepositoryDelegatorFunction.java | 84 ++- .../lib/runtime/RemoteRepoContentsCache.java | 45 ++ .../RepositoryRemoteHelpersFactory.java | 6 +- .../skyframe/ActionOutputMetadataStore.java | 17 +- .../build/lib/skyframe/ArtifactFunction.java | 7 + .../build/lib/skyframe/FileStateFunction.java | 16 +- .../build/lib/skyframe/PackageFunction.java | 60 +- .../SkyframeTargetPatternEvaluator.java | 5 + .../com/google/devtools/build/lib/vfs/BUILD | 3 + .../build/lib/vfs/DetailedIOException.java | 53 ++ src/main/protobuf/failure_details.proto | 1 + .../google/devtools/build/lib/remote/BUILD | 2 + .../build/lib/remote/GrpcCacheClientTest.java | 3 +- .../remote/RemoteActionInputFetcherTest.java | 9 +- .../remote/RemoteExecutionServiceTest.java | 3 +- .../build/lib/remote/RemoteModuleTest.java | 10 + .../lib/remote/RemoteSpawnRunnerTest.java | 3 +- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 3 +- .../build/lib/remote/UploadManifestTest.java | 98 +++ src/test/py/bazel/BUILD | 12 + .../bzlmod/remote_repo_contents_cache_test.py | 622 ++++++++++++++++ src/test/py/bazel/test_base.py | 9 +- .../build/remote/worker/ExecutionServer.java | 3 +- 45 files changed, 2564 insertions(+), 169 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java create mode 100644 src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java rename src/main/java/com/google/devtools/build/lib/remote/{RemoteRepositoryHelpersFactory.java => RepositoryRemoteHelpersFactoryImpl.java} (74%) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java create mode 100644 src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java create mode 100644 src/main/java/com/google/devtools/build/lib/vfs/DetailedIOException.java create mode 100644 src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc index 247baf7e8bf0a2..a12ce976a79b54 100644 --- a/src/main/cpp/blaze.cc +++ b/src/main/cpp/blaze.cc @@ -548,6 +548,11 @@ static vector GetServerExeArgs(const blaze_util::Path &jvm_path, } else { result.push_back("--nowindows_enable_symlinks"); } + if (startup_options.remote_repo_contents_cache) { + result.push_back("--experimental_remote_repo_contents_cache"); + // Don't set the flag to false if it's not set - non-OSS Blaze does not know + // about this flag. + } // We use this syntax so that the logic in AreStartupOptionsDifferent() that // decides whether the server needs killing is simpler. This is parsed by // the Java code where --noclient_debug and --client_debug=false are diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc index 78324c987e017d..38297f7d2dd705 100644 --- a/src/main/cpp/startup_options.cc +++ b/src/main/cpp/startup_options.cc @@ -64,7 +64,7 @@ void StartupOptions::OverrideOptionSourcesKey(const std::string &flag_name, option_sources_key_override_[flag_name] = new_name; } -StartupOptions::StartupOptions(const string &product_name, +StartupOptions::StartupOptions(const string& product_name, bool lock_install_base) : product_name(product_name), lock_install_base(lock_install_base), @@ -101,7 +101,8 @@ StartupOptions::StartupOptions(const string &product_name, cgroup_parent(), run_in_user_cgroup(false), #endif - windows_enable_symlinks(false) { + windows_enable_symlinks(false), + remote_repo_contents_cache(false) { #if defined(_WIN32) || defined(__CYGWIN__) string windows_unix_root = DetectBashAndExportBazelSh(); if (!windows_unix_root.empty()) { @@ -136,6 +137,8 @@ StartupOptions::StartupOptions(const string &product_name, RegisterNullaryStartupFlag("write_command_log", &write_command_log); RegisterNullaryStartupFlag("windows_enable_symlinks", &windows_enable_symlinks); + RegisterNullaryStartupFlag("experimental_remote_repo_contents_cache", + &remote_repo_contents_cache); #ifdef __linux__ RegisterNullaryStartupFlag("experimental_run_in_user_cgroup", &run_in_user_cgroup); diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h index 4efef10b7f9673..45c0500fb1f3a0 100644 --- a/src/main/cpp/startup_options.h +++ b/src/main/cpp/startup_options.h @@ -294,6 +294,10 @@ class StartupOptions { // developer mode to be enabled. bool windows_enable_symlinks; + // Whether to use a remote cache to store the contents of reproducible + // external repositories. + bool remote_repo_contents_cache; + protected: // Constructor for subclasses only so that site-specific extensions of this // class can override the product name. The product_name must be capitalized, diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index a2e6752c240a89..82f62b05e62f58 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -281,6 +281,16 @@ java_library( ], ) +java_library( + name = "runtime/remote_repo_contents_cache", + srcs = ["runtime/RemoteRepoContentsCache.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/vfs", + ], +) + java_library( name = "runtime", srcs = glob( @@ -301,6 +311,7 @@ java_library( "runtime/MemoryPressureEvent.java", "runtime/MemoryPressureOptions.java", "runtime/MemoryPressureStatCollector.java", + "runtime/RemoteRepoContentsCache.java", "runtime/StarlarkOptionsParser.java", "runtime/TestSummaryOptions.java", ], @@ -314,6 +325,7 @@ java_library( ":runtime/command_dispatcher", ":runtime/command_line_path_factory", ":runtime/memory_pressure", + ":runtime/remote_repo_contents_cache", ":runtime/test_summary_options", ":starlark_options_parser", "//src/main/java/com/google/devtools/build/lib/actions", diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java index 3a9812aa907d7f..abea5d93d9c361 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java @@ -153,6 +153,9 @@ private static FileStateValue createRegularFileStateValueFromPath( throws InconsistentFilesystemException { checkState(stat.isFile(), path); + if (stat instanceof FileStatusWithMetadata fileStatusWithMetadata) { + return new RegularFileStateValueWithMetadata(fileStatusWithMetadata.getMetadata()); + } try { // If the digest will be injected, we can skip calling getFastDigest, but we need to store a // contents proxy because if the digest is injected but is not available from the filesystem, @@ -386,6 +389,77 @@ public String prettyPrint() { } } + /** + * Implementation of {@link FileStateValue} for regular files when its metadata is backed by a + * {@link FileArtifactValue}. + */ + public static final class RegularFileStateValueWithMetadata extends FileStateValue { + private final FileArtifactValue metadata; + + @VisibleForTesting + public RegularFileStateValueWithMetadata(FileArtifactValue metadata) { + this.metadata = checkNotNull(metadata); + } + + @Override + public FileStateType getType() { + return FileStateType.REGULAR_FILE; + } + + @Override + public long getSize() { + return metadata.getSize(); + } + + @Override + @Nullable + public byte[] getDigest() { + return metadata.getDigest(); + } + + @Override + public FileContentsProxy getContentsProxy() { + return metadata.getContentsProxy(); + } + + public FileArtifactValue getMetadata() { + return metadata; + } + + @Override + public boolean equals(Object obj) { + if (obj == this) { + return true; + } + if (!(obj instanceof RegularFileStateValueWithMetadata other)) { + return false; + } + return other.metadata.equals(this.metadata); + } + + @Override + public int hashCode() { + return metadata.hashCode(); + } + + @Override + public byte[] getValueFingerprint() { + Fingerprint fp = new Fingerprint().addLong(getSize()); + fp.addBytes(getDigest()); + return fp.digestAndReset(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("metadata", metadata).toString(); + } + + @Override + public String prettyPrint() { + return String.format("regular file with size of %d and %s", getSize(), metadata); + } + } + /** Implementation of {@link FileStateValue} for special files that exist. */ @VisibleForTesting public static final class SpecialFileStateValue extends FileStateValue { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD index 937b912016915c..6dc818b4c6df12 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -51,6 +51,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", "//src/main/java/com/google/devtools/build/lib/rules:repository/new_local_repository_function", "//src/main/java/com/google/devtools/build/lib/rules:repository/new_local_repository_rule", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index b169f8ded8df87..77af5a4afb76eb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -99,6 +99,7 @@ import com.google.devtools.build.lib.runtime.CommonCommandOptions; import com.google.devtools.build.lib.runtime.InfoItem; import com.google.devtools.build.lib.runtime.ProcessWrapper; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; import com.google.devtools.build.lib.runtime.ServerBuilder; @@ -151,6 +152,7 @@ public class BazelRepositoryModule extends BlazeModule { private final ImmutableMap repositoryHandlers; private final AtomicBoolean isFetch = new AtomicBoolean(false); private final StarlarkRepositoryFunction starlarkRepositoryFunction; + private RepositoryDelegatorFunction repositoryDelegatorFunction; private final RepositoryCache repositoryCache = new RepositoryCache(); private final MutableSupplier> repoEnvironmentSupplier = new MutableSupplier<>(); @@ -249,7 +251,7 @@ public void workspaceInit( } // Create the repository function everything flows through. - RepositoryDelegatorFunction repositoryDelegatorFunction = + repositoryDelegatorFunction = new RepositoryDelegatorFunction( repositoryHandlers, starlarkRepositoryFunction, @@ -658,10 +660,13 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { RepositoryRemoteHelpersFactory repositoryRemoteHelpersFactory = env.getRuntime().getRepositoryHelpersFactory(); RepositoryRemoteExecutor remoteExecutor = null; + RemoteRepoContentsCache remoteRepoContentsCache = null; if (repositoryRemoteHelpersFactory != null) { remoteExecutor = repositoryRemoteHelpersFactory.createExecutor(); + remoteRepoContentsCache = repositoryRemoteHelpersFactory.createRepoContentsCache(); } starlarkRepositoryFunction.setRepositoryRemoteExecutor(remoteExecutor); + repositoryDelegatorFunction.setRemoteRepoContentsCache(remoteRepoContentsCache); singleExtensionEvalFunction.setRepositoryRemoteExecutor(remoteExecutor); clock = env.getClock(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index 76061bc9d60753..6a9e7af5fb3af5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -34,6 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/repository:repository_events", "//src/main/java/com/google/devtools/build/lib/repository:request_repository_information_event", "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index f5e704b5e73e47..17f3d25a6fbad7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem; import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.Dirents; @@ -2315,6 +2316,19 @@ private StarlarkPath findCommandOnPath(String program) throws IOException { // Resolve the label given by value into a file path. protected StarlarkPath getPathFromLabel(Label label) throws EvalException, InterruptedException { RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env); + if (rootedPath == null) { + throw new NeedsSkyframeRestartException(); + } + if (!label.getRepository().isMain() + && directories.getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + try { + remoteFs.ensureMaterialized(label.getRepository(), env.getListener()); + } catch (IOException e) { + throw Starlark.errorf( + "Failed to materialize remote repo %s: %s", label.getRepository(), e.getMessage()); + } + } StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath()); try { maybeWatch( diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 2fc08a35ed53c2..ae348d0b3510f9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -26,6 +26,8 @@ import static java.util.Objects.requireNonNull; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; @@ -50,6 +52,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.FileSymlinkLoopException; @@ -80,10 +83,15 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet private final TempPathGenerator tempPathGenerator; private final OutputPermissions outputPermissions; - protected final Path execRoot; + // The exec root may only be resolved once the workspace name is known, which does not happen + // until the loading phase. Since the remote repo contents cache materializes external repos + // before then (and only ever resolves paths under the external directory, which lives directly + // under the output base), exec root resolution is deferred via this supplier. + private final Supplier execRootSupplier; + protected final Path outputBase; protected final RemoteOutputChecker remoteOutputChecker; - protected final ActionOutputDirectoryHelper outputDirectoryHelper; + @Nullable protected final ActionOutputDirectoryHelper outputDirectoryHelper; /** The state of a directory tracked by {@link DirectoryTracker}, as explained below. */ enum DirectoryState { @@ -124,6 +132,17 @@ void setPermanentlyWritable(Path dir) throws IOException { } private void setWritable(Path dir, DirectoryState newState) throws IOException { + // External repo paths (which live directly under the output base) are not build outputs and + // don't need output permission management. Check this first (comparing fragments, since the + // dir may be on the host file system while the output base is on an overlay) so that the exec + // root, which is only resolvable during the loading phase and later, is not resolved during + // external repo materialization. + if (dir.asFragment() + .startsWith( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment()) + || !dir.startsWith(execRoot())) { + return; + } AtomicReference caughtException = new AtomicReference<>(); directoryStateMap.compute( @@ -136,7 +155,11 @@ private void setWritable(Path dir, DirectoryState newState) throws IOException { return newState == DirectoryState.PERMANENTLY_WRITABLE ? newState : oldState; } try { - outputDirectoryHelper.createOutputDirectory(dir, execRoot); + if (outputDirectoryHelper != null) { + outputDirectoryHelper.createOutputDirectory(dir, execRoot()); + } else { + dir.createDirectoryAndParents(); + } dir.setWritable(true); } catch (IOException e) { caughtException.set(e); @@ -202,19 +225,48 @@ static Symlink of(PathFragment linkExecPath, PathFragment targetExecPath) { protected AbstractActionInputPrefetcher( Reporter reporter, - Path execRoot, + Supplier execRootSupplier, + Path outputBase, TempPathGenerator tempPathGenerator, RemoteOutputChecker remoteOutputChecker, - ActionOutputDirectoryHelper outputDirectoryHelper, + @Nullable ActionOutputDirectoryHelper outputDirectoryHelper, OutputPermissions outputPermissions) { this.reporter = reporter; - this.execRoot = execRoot; + this.execRootSupplier = Suppliers.memoize(execRootSupplier::get); + this.outputBase = outputBase; this.tempPathGenerator = tempPathGenerator; this.remoteOutputChecker = remoteOutputChecker; this.outputDirectoryHelper = outputDirectoryHelper; this.outputPermissions = outputPermissions; } + /** + * Returns the exec root. May only be called once the workspace name is known, i.e. not before the + * loading phase. Code paths reached during external repository materialization must use {@link + * #outputBase} instead. + */ + protected Path execRoot() { + return execRootSupplier.get(); + } + + /** + * Resolves an exec path to an absolute path. On 8.7.0, external repos are at + * output_base/external/ (sibling of execroot/), so exec paths starting with "external/" must be + * resolved relative to the output base rather than the exec root. + * + *

Absolute paths (e.g., from RemoteExternalOverlayFileSystem.prefetch) are returned as-is on + * the exec root's file system. + */ + private Path resolveExecPath(PathFragment execPath) { + if (execPath.isAbsolute()) { + return outputBase.getFileSystem().getPath(execPath); + } + if (execPath.startsWith(LabelConstants.EXTERNAL_REPOSITORY_LOCATION)) { + return outputBase.getRelative(execPath); + } + return execRoot().getRelative(execPath); + } + private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) throws IOException { var stat = path.statIfFound(); @@ -228,7 +280,12 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) if (stat.getSize() != metadata.getSize()) { return true; } - var contentsProxy = metadata.getContentsProxy(); + FileContentsProxy contentsProxy; + try { + contentsProxy = metadata.getContentsProxy(); + } catch (UnsupportedOperationException e) { + contentsProxy = null; + } if (contentsProxy != null && contentsProxy.equals(FileContentsProxy.create(stat))) { return false; } @@ -254,7 +311,7 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) * @param tempPath the temporary path which the input should be written to. */ protected abstract ListenableFuture doDownloadFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -282,11 +339,36 @@ public ListenableFuture prefetchFiles( MetadataSupplier metadataSupplier, Priority priority, Reason reason) { + return prefetchFilesInterruptibly(action, inputs, metadataSupplier, priority, reason); + } + + /** + * Fetches remotely stored action outputs and stores them under their path in the output base. + * + *

The {@code inputs} may not contain any unexpanded directories. + * + *

This method is safe to be called concurrently from spawn runners before running any local + * spawn. + * + *

This method is similar to #prefetchFiles() above, but note that {@code metadataSupplier} may + * throw {@link InterruptedException}. If it does, this method will propagate this exception in + * the returned future. + * + * @return a future that is completed once all downloads have finished. + */ + public ListenableFuture prefetchFilesInterruptibly( + @Nullable ActionExecutionMetadata action, + Iterable inputs, + MetadataSupplier metadataSupplier, + Priority priority, + Reason reason) { List files = new ArrayList<>(); for (ActionInput input : inputs) { - // Source artifacts don't need to be fetched. - if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) { + // Source artifacts in the main repo don't need to be fetched. + if (input instanceof Artifact artifact + && artifact.isSourceArtifact() + && artifact.getArtifactOwner().getLabel().getRepository().isMain()) { continue; } @@ -343,7 +425,7 @@ public ListenableFuture prefetchFiles( } private ListenableFuture prefetchFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Set dirsWithOutputPermissions, MetadataSupplier metadataSupplier, ActionInput input, @@ -358,7 +440,7 @@ private ListenableFuture prefetchFile( PathFragment execPath = input.getExecPath(); FileArtifactValue metadata = metadataSupplier.getMetadata(input); - if (metadata == null || !canDownloadFile(execRoot.getRelative(execPath), metadata)) { + if (metadata == null || !canDownloadFile(resolveExecPath(execPath), metadata)) { return immediateVoidFuture(); } @@ -375,8 +457,8 @@ private ListenableFuture prefetchFile( Completable result = downloadFileNoCheckRx( action, - execRoot.getRelative(execPath), - treeRootExecPath != null ? execRoot.getRelative(treeRootExecPath) : null, + resolveExecPath(execPath), + treeRootExecPath != null ? resolveExecPath(treeRootExecPath) : null, dirsWithOutputPermissions, input, metadata, @@ -504,7 +586,7 @@ private Path maybeResolveSymlink(Path path) throws IOException { } private Completable downloadFileNoCheckRx( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Path path, @Nullable Path treeRoot, Set dirsWithOutputPermissions, @@ -543,8 +625,19 @@ private Completable downloadFileNoCheckRx( } } - Path finalPath = path; - PathFragment execPath = finalPath.relativeTo(execRoot); + // Downloads should always be written to the "actual" host file system, not any overlays. + Path finalPath = path.forHostFileSystem(); + PathFragment execPath; + if (finalPath + .asFragment() + .startsWith(outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { + // On 8.7.0, external repos are at output_base/external/ which is not under execRoot + // (output_base/execroot/_main/). Use the path relative to the output base instead. This also + // avoids resolving the exec root during external repo materialization, before it is known. + execPath = finalPath.asFragment().relativeTo(outputBase.asFragment()); + } else { + execPath = finalPath.asFragment().relativeTo(execRoot().asFragment()); + } Completable download = usingTempPath( @@ -552,12 +645,21 @@ private Completable downloadFileNoCheckRx( toCompletable( () -> doDownloadFile( - action, reporter, tempPath, execPath, metadata, priority, reason), + action, + reporter, + tempPath.forHostFileSystem(), + execPath, + metadata, + priority, + reason), directExecutor()) .doOnComplete( () -> { finalizeDownload( - metadata, tempPath, finalPath, dirsWithOutputPermissions); + metadata, + tempPath.forHostFileSystem(), + finalPath, + dirsWithOutputPermissions); alreadyDeleted.set(true); }) .onErrorResumeNext( @@ -590,26 +692,34 @@ private void finalizeDownload( throws IOException { Path parentDir = checkNotNull(finalPath.getParentDirectory()); - // Ensure the parent directory exists and is writable. We cannot rely on this precondition to be - // have been established by the execution of the owning action in a previous invocation, since - // the output tree may have been externally modified in between invocations. - if (dirsWithOutputPermissions.contains(parentDir)) { - // The file belongs to a tree artifact created by an action that declared an output directory - // (as opposed to an action template expansion). The output permissions should be set on the - // parent directory after prefetching. - directoryTracker.setTemporarilyWritable(parentDir); + // External repo paths live directly under the output base, not the exec root. Classify by the + // external directory (rather than the exec root, which isn't resolvable during external repo + // materialization) and compare as fragments since the exec root may be located on a file system + // overlaying the host file system where the download is written to. + if (!finalPath + .asFragment() + .startsWith(outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { + // Ensure the parent directory exists and is writable. We cannot rely on this precondition to + // have been established by the execution of the owning action in a previous invocation, since + // the output tree may have been externally modified in between invocations. + if (dirsWithOutputPermissions.contains(parentDir)) { + // The file belongs to a tree artifact created by an action that declared an output + // directory (as opposed to an action template expansion). The output permissions should be + // set on the parent directory after prefetching. + directoryTracker.setTemporarilyWritable(parentDir); + } else { + // One of the following must apply: + // (1) The file does not belong to a tree artifact. + // (2) The file belongs to a tree artifact created by an action template expansion. + // In case (1), the parent directory is a package or a subdirectory of a package, and should + // remain writable. In case (2), even though we arguably ought to set the output permissions + // on the parent directory to match local execution, we choose not to do it and avoid the + // additional implementation complexity required to detect a race condition between + // concurrent calls touching the same directory. + directoryTracker.setPermanentlyWritable(parentDir); + } } else { - // There are three cases: - // (1) The file does not belong to a tree artifact. - // (2) The file belongs to a tree artifact created by an action template expansion. - // (3) The file belongs to a tree artifact but we don't know it. This can occur when the - // file belongs to a tree artifact inside a fileset (see b/254844173). - // In case (1), the parent directory is a package or a subdirectory of a package, and should - // remain writable. In cases (2) and (3), even though we arguably ought to set the output - // permissions on the parent directory to match the outcome of a locally executed action, we - // choose not to do it and avoid the additional implementation complexity required to detect a - // race condition between concurrent calls touching the same directory. - directoryTracker.setPermanentlyWritable(parentDir); + parentDir.createDirectoryAndParents(); } // Set output permissions on files, matching the behavior of SkyframeActionExecutor#checkOutputs @@ -656,18 +766,18 @@ private void deletePartialDownload(Path path) { private Completable plantSymlink(Symlink symlink) { return downloadCache.execute( - execRoot.getRelative(symlink.linkExecPath()), + resolveExecPath(symlink.linkExecPath()), Completable.defer( () -> { - Path link = execRoot.getRelative(symlink.linkExecPath()); - Path target = execRoot.getRelative(symlink.targetExecPath()); + Path link = resolveExecPath(symlink.linkExecPath()); + Path target = resolveExecPath(symlink.targetExecPath()); // Delete the link path if it already exists. This is the case for tree artifacts, // whose root directory is created before the action runs. link.delete(); link.createSymbolicLink(target); return Completable.complete(); }), - forceRefetch(execRoot.getRelative(symlink.linkExecPath()))); + forceRefetch(resolveExecPath(symlink.linkExecPath()))); } public ImmutableSet downloadedFiles() { diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index ab43618315ca7c..c00dcb0d86766f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -63,6 +63,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:build-request-options", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:runtime/command_line_path_factory", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", @@ -72,6 +73,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_tree", "//src/main/java/com/google/devtools/build/lib/actions:forbidden_action_input_exception", + "//src/main/java/com/google/devtools/build/lib/actions:important_output_handler", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", @@ -86,6 +88,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/clock", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:abstract_spawn_strategy", @@ -119,6 +122,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/unsafe:string", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", @@ -244,6 +248,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:action_output_directory_helper", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 7b6bd09109a5a8..026887e5d0c26c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -149,6 +149,10 @@ public void setActionInputFetcher(RemoteActionInputFetcher actionInputFetcher) { this.actionInputFetcher = actionInputFetcher; } + public RemoteActionInputFetcher getActionInputFetcher() { + return checkNotNull(actionInputFetcher); + } + private RemoteExecutionService getRemoteExecutionService() { if (remoteExecutionService == null) { Path workingDirectory = env.getWorkingDirectory(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index 45af579e91378a..7df167f8bd19f7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -17,12 +17,14 @@ import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.base.Preconditions; +import com.google.common.base.Supplier; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionOutputDirectoryHelper; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -39,8 +41,9 @@ /** * Stages output files that are stored remotely to the local filesystem. * - *

This is necessary when a locally executed action consumes outputs produced by a remotely - * executed action and {@code --experimental_remote_download_outputs=minimal} is specified. + *

This is used to ensure that the inputs to a local action are present, even when they are + * provided by a remote action when building without the bytes, or by an external repository when + * building with a remote repository cache enabled. */ public class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { @@ -54,14 +57,16 @@ public class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { String buildRequestId, String commandId, CombinedCache combinedCache, - Path execRoot, + Supplier execRootSupplier, + Path outputBase, TempPathGenerator tempPathGenerator, RemoteOutputChecker remoteOutputChecker, - ActionOutputDirectoryHelper outputDirectoryHelper, + @Nullable ActionOutputDirectoryHelper outputDirectoryHelper, OutputPermissions outputPermissions) { super( reporter, - execRoot, + execRootSupplier, + outputBase, tempPathGenerator, remoteOutputChecker, outputDirectoryHelper, @@ -78,7 +83,7 @@ public boolean requiresTreeMetadataWhenTreeFileIsInput() { @Override protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException { - input.atomicallyWriteRelativeTo(execRoot); + input.atomicallyWriteRelativeTo(execRoot()); } @Override @@ -92,14 +97,23 @@ protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { @Override protected boolean forceRefetch(Path path) { + // External repo paths are never rewound action outputs. Check this first (comparing fragments, + // since the path may be on the host file system while the output base is on an overlay) to avoid + // resolving the exec root during external repo materialization, before it is known. + if (path.asFragment() + .startsWith( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { + return false; + } // Caches for download operations and output directory creation need to be disregarded for the // outputs of rewound actions as they may have been deleted after they were first created. + Path execRoot = execRoot(); return path.startsWith(execRoot) && rewoundActionOutputs.contains(path.relativeTo(execRoot)); } @Override protected ListenableFuture doDownloadFile( - ActionExecutionMetadata action, + @Nullable ActionExecutionMetadata action, Reporter reporter, Path tempPath, PathFragment execPath, @@ -127,7 +141,11 @@ protected ListenableFuture doDownloadFile( tempPath, digest, new CombinedCache.DownloadProgressReporter( - progress -> progress.postTo(reporter, action), + progress -> { + if (action != null) { + progress.postTo(reporter, action); + } + }, execPath.toString(), digest.getSizeBytes())); } @@ -138,7 +156,9 @@ public void handleRewoundActionOutputs(Collection outputs) { // true. While it is true that resetting outputDirectoryHelper isn't necessary to undo the // caching of output directory creation during action preparation, we still need to reset here // since outputDirectoryHelper is also used by AbstractActionInputPrefetcher. - outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(outputs); + if (outputDirectoryHelper != null) { + outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(outputs); + } for (Artifact output : outputs) { // Action templates have TreeFileArtifacts as outputs, which isn't supported by the trie. We // only need to track the tree artifacts themselves. diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index ec6b9b0054e872..c9dd30af0372b1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1831,18 +1831,18 @@ UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) return UploadManifest.create( remoteOptions, - combinedCache.getRemoteCacheCapabilities(), - digestUtil, - action.getRemotePathResolver(), - action.getActionKey(), - action.getAction(), - action.getCommand(), - outputFiles.build(), - action.getSpawnExecutionContext().getFileOutErr(), - spawnResult.exitCode(), - spawnResult.getStartTime(), - spawnResult.getWallTimeInMs()); - + combinedCache.getRemoteCacheCapabilities(), + digestUtil, + action.getRemotePathResolver(), + action.getActionKey(), + action.getAction(), + action.getCommand(), + outputFiles.build(), + action.getSpawnExecutionContext().getFileOutErr(), + spawnResult.exitCode(), + spawnResult.getStartTime(), + spawnResult.getWallTimeInMs(), + /* preserveExecutableBit= */ false); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java new file mode 100644 index 00000000000000..fac4b90300b74e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java @@ -0,0 +1,677 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote; + +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.util.concurrent.Futures.immediateCancelledFuture; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; +import static com.google.devtools.build.lib.util.StringEncoding.unicodeToInternal; + +import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.Directory; +import build.bazel.remote.execution.v2.Tree; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.remote.common.BulkTransferException; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.remote.util.Utils; +import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.skyframe.SkyFunctions; +import com.google.devtools.build.lib.vfs.DetailedIOException; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.SymlinkTargetType; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.skyframe.MemoizingEvaluator; +import com.google.devtools.build.skyframe.SkyFunctionException; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.InterruptedIOException; +import java.io.OutputStream; +import java.nio.channels.SeekableByteChannel; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import javax.annotation.Nullable; + +/** + * A file system that overlays the native file system with a {@link RemoteExternalFileSystem} for + * the "external" directory, which contains the contents of external repositories. + * + *

Each external repository can either be materialized to the native file system or kept in + * memory in the {@link RemoteExternalFileSystem}. + */ +public final class RemoteExternalOverlayFileSystem extends FileSystem { + + @Override + public boolean isFilePathCaseSensitive() { + return nativeFs.isFilePathCaseSensitive(); + } + + @Override + public FileSystem getHostFileSystem() { + return nativeFs.getHostFileSystem(); + } + + private final PathFragment externalDirectory; + private final int externalDirectorySegmentCount; + private final FileSystem nativeFs; + private final RemoteExternalFileSystem externalFs; + private final ConcurrentHashMap> materializations = + new ConcurrentHashMap<>(); + // As long as a repo name appears as a key in this map, the repo contents are available in + // externalFs. + private final ConcurrentHashMap markerFileContents = new ConcurrentHashMap<>(); + private final Set reposWithLostFiles = ConcurrentHashMap.newKeySet(); + + // Per-build information that is set in beforeCommand and cleared in afterCommand. + @Nullable private CombinedCache cache; + @Nullable private AbstractActionInputPrefetcher inputPrefetcher; + @Nullable private Reporter reporter; + @Nullable private String buildRequestId; + @Nullable private String commandId; + @Nullable private MemoizingEvaluator evaluator; + @Nullable private ExecutorService materializationExecutor; + + public RemoteExternalOverlayFileSystem(PathFragment externalDirectory, FileSystem nativeFs) { + super(nativeFs.getDigestFunction()); + this.externalDirectory = externalDirectory; + this.externalDirectorySegmentCount = externalDirectory.segmentCount(); + this.nativeFs = nativeFs; + this.externalFs = new RemoteExternalFileSystem(nativeFs.getDigestFunction()); + } + + @SuppressWarnings("AllowVirtualThreads") + public void beforeCommand( + CombinedCache cache, + AbstractActionInputPrefetcher inputPrefetcher, + Reporter reporter, + String buildRequestId, + String commandId, + MemoizingEvaluator evaluator) { + checkState( + this.cache == null + && this.inputPrefetcher == null + && this.reporter == null + && this.buildRequestId == null + && this.commandId == null + && this.evaluator == null + && this.materializationExecutor == null); + this.cache = cache; + this.inputPrefetcher = inputPrefetcher; + this.reporter = reporter; + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.evaluator = evaluator; + this.materializationExecutor = + Executors.newThreadPerTaskExecutor( + Thread.ofVirtual().name("remote-repo-materialization-", 0).factory()); + } + + public void afterCommand() { + this.cache = null; + this.inputPrefetcher = null; + this.reporter = null; + this.buildRequestId = null; + this.commandId = null; + // Materializations happen synchronously and upon request by other repo rules, so there is no + // reason to await their orderly completion in afterCommand. + materializationExecutor.shutdownNow(); + materializationExecutor = null; + // Clean up the in-memory contents of materialized repos to save memory, or those that need to + // be refetched to recover files that the remote cache has lost. This wouldn't be safe to do + // eagerly as ongoing repo rule evaluations may still refer to the in-memory content and + // refetching is not atomic. + materializations.forEach( + 1, + (repoName, materializationState) -> + materializationState.state() == Future.State.SUCCESS + || reposWithLostFiles.contains(repoName) + ? repoName + : null, + repoName -> { + try { + externalFs.getPath(externalDirectory.getChild(repoName)).deleteTree(); + } catch (IOException e) { + throw new IllegalStateException("In-memory file system is not expected to throw", e); + } + materializations.remove(repoName); + markerFileContents.remove(repoName); + }); + if (!reposWithLostFiles.isEmpty()) { + evaluator.delete( + k -> + k.functionName().equals(SkyFunctions.REPOSITORY_DIRECTORY) + && reposWithLostFiles.contains(((RepositoryName) k.argument()).getName())); + } + reposWithLostFiles.clear(); + this.evaluator = null; + } + + public void injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile) + throws IOException { + var childMap = + remoteContents.getChildrenList().stream() + .collect( + toImmutableMap(cache.digestUtil::compute, directory -> directory, (a, b) -> a)); + injectRecursively( + externalFs, externalDirectory.getChild(repo.getName()), remoteContents.getRoot(), childMap); + // Create the repo directory on disk so that readdir reflects the overlaid state of the external + // directory. + nativeFs.createDirectoryAndParents(externalDirectory.getChild(repo.getName())); + // Keep the marker file contents in memory so that it can be written out when the repo is + // materialized. This doubles as a presence marker for the in-memory repo contents. + markerFileContents.put(repo.getName(), markerFile); + } + + private static void injectRecursively( + RemoteExternalFileSystem fs, + PathFragment path, + Directory dir, + ImmutableMap childMap) + throws IOException { + fs.createDirectoryAndParents(path); + for (var file : dir.getFilesList()) { + var filePath = path.getRelative(unicodeToInternal(file.getName())); + fs.injectFile( + filePath, + FileArtifactValue.RemoteFileArtifactValue.create( + DigestUtil.toBinaryDigest(file.getDigest()), + file.getDigest().getSizeBytes(), + /* locationIndex= */ 1)); + fs.getPath(filePath).setExecutable(file.getIsExecutable()); + // The RE API does not track whether a file is readable or writable. We choose to make all + // files readable and not writable to ensure that other repo rules can't accidentally modify + // the cached repo. + fs.getPath(filePath).setWritable(false); + } + for (var symlink : dir.getSymlinksList()) { + fs.getPath(path.getRelative(unicodeToInternal(symlink.getName()))) + .createSymbolicLink(PathFragment.create(unicodeToInternal(symlink.getTarget()))); + } + for (var subdirNode : dir.getDirectoriesList()) { + var subdirPath = path.getRelative(unicodeToInternal(subdirNode.getName())); + var subdir = childMap.get(subdirNode.getDigest()); + if (subdir == null) { + throw new IOException( + "Directory %s with digest %s not found in tree" + .formatted(subdirPath, subdirNode.getDigest().getHash())); + } + injectRecursively(fs, subdirPath, subdir, childMap); + } + } + + /** + * Materializes the given external repository to the native file system if it hasn't been + * materialized yet. This method blocks until the materialization is complete. + * + *

This should only be used for cases in which the given repo is accessed non-hermetically, + * such as when another repo rule that depends on its files executes a command. Selective reads by + * Bazel or local actions are handled automatically by the file system or {@link + * AbstractActionInputPrefetcher}. + */ + public void ensureMaterialized(RepositoryName repo, ExtendedEventHandler reporter) + throws IOException, InterruptedException { + if (!markerFileContents.containsKey(repo.getName())) { + // The repo has not been injected into the in-memory file system. + return; + } + var unused = + getFromFuture( + materializations.computeIfAbsent( + repo.getName(), + unusedRepoName -> + materializationExecutor.submit( + () -> { + doMaterialize(repo, reporter); + return null; + }))); + } + + private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) + throws IOException, InterruptedException { + reporter.handle(Event.debug("Materializing remote repo %s".formatted(repo))); + var repoPath = externalDirectory.getChild(repo.getName()); + var remoteRepo = externalFs.getPath(repoPath); + var walkResult = walk(remoteRepo); + var unused = + getFromFuture( + inputPrefetcher.prefetchFilesInterruptibly( + /* action= */ null, + Iterables.transform( + walkResult.files(), path -> ActionInputHelper.fromPath(path.asFragment())), + actionInput -> externalFs.getMetadata(actionInput.getExecPath()), + ActionInputPrefetcher.Priority.CRITICAL, + ActionInputPrefetcher.Reason.INPUTS)); + // Create symlinks last as some platforms don't allow creating a symlink to a non-existent + // target. + for (var remoteSymlink : walkResult.symlinks()) { + var nativeSymlink = nativeFs.getPath(remoteSymlink.asFragment()); + nativeSymlink.getParentDirectory().createDirectoryAndParents(); + nativeSymlink.createSymbolicLink(remoteSymlink.readSymbolicLink()); + } + + // After the repo has been copied, atomically materialize the marker file. This ensures that the + // repo doesn't have to be refetched after the next server restart. + var markerFile = nativeFs.getPath(externalDirectory.getChild(repo.getMarkerFileName())); + var markerFileSibling = + nativeFs.getPath(externalDirectory.getChild(repo.getMarkerFileName() + ".tmp")); + FileSystemUtils.writeContentAsLatin1( + markerFileSibling, markerFileContents.remove(repo.getName())); + markerFileSibling.renameTo(markerFile); + } + + private record WalkResult(List files, List symlinks) {} + + private static WalkResult walk(Path root) throws IOException { + var result = new WalkResult(new ArrayList<>(), new ArrayList<>()); + walk(root, result); + return result; + } + + private static void walk(Path root, WalkResult result) throws IOException { + for (var dirent : root.readdir(Symlinks.NOFOLLOW)) { + var fromChild = root.getChild(dirent.getName()); + switch (dirent.getType()) { + case FILE -> result.files.add(fromChild); + case SYMLINK -> result.symlinks.add(fromChild); + case DIRECTORY -> walk(fromChild, result); + default -> throw new IOException("Unsupported file type: " + dirent); + } + } + } + + // Always mirror tree deletions to the underlying native file system to support bazel clean and + // repository refetching. + + @Override + public void deleteTree(PathFragment path) throws IOException { + nativeFs.getPath(path).deleteTree(); + externalFs.getPath(path).deleteTree(); + } + + @Override + public void deleteTreesBelow(PathFragment dir) throws IOException { + nativeFs.getPath(dir).deleteTreesBelow(); + externalFs.getPath(dir).deleteTreesBelow(); + } + + // All other methods delegate to the file system given by this method. It is important to override + // each non-final FileSystem method to benefit from optimizations implemented in the respective + // underlying file systems. + private FileSystem fsForPath(PathFragment path) { + if (path.startsWith(externalDirectory) && !path.equals(externalDirectory)) { + String repoName = path.getSegment(externalDirectorySegmentCount); + var hasBeenInjected = markerFileContents.containsKey(repoName); + var hasBeenMaterialized = + materializations.getOrDefault(repoName, immediateCancelledFuture()).state() + == Future.State.SUCCESS; + if (hasBeenInjected && !hasBeenMaterialized) { + // The repo may have been deleted due to refetching. Clean up in-memory state if that is the + // case. + if (externalFs.getPath(externalDirectory.getChild(repoName)).exists()) { + return externalFs; + } + materializations.remove(repoName); + markerFileContents.remove(repoName); + } + // Fall back to the native file system if the repo has been materialized, deleted, or never + // injected. + } + return nativeFs; + } + + @Override + public boolean delete(PathFragment path) throws IOException { + return fsForPath(path).delete(path); + } + + @Override + public byte[] getDigest(PathFragment path) throws IOException { + return fsForPath(path).getDigest(path); + } + + @Nullable + @Override + public byte[] getFastDigest(PathFragment path) throws IOException { + return fsForPath(path).getFastDigest(path); + } + + @Override + public boolean supportsModifications(PathFragment path) { + return fsForPath(path).supportsModifications(path); + } + + @Override + public boolean supportsSymbolicLinksNatively(PathFragment path) { + return fsForPath(path).supportsSymbolicLinksNatively(path); + } + + @Override + public boolean supportsHardLinksNatively(PathFragment path) { + return fsForPath(path).supportsHardLinksNatively(path); + } + + @Override + public boolean createDirectory(PathFragment path) throws IOException { + return fsForPath(path).createDirectory(path); + } + + @Override + public void createDirectoryAndParents(PathFragment path) throws IOException { + fsForPath(path).createDirectoryAndParents(path); + } + + @Override + public long getFileSize(PathFragment path, boolean followSymlinks) throws IOException { + return fsForPath(path).getFileSize(path, followSymlinks); + } + + @Override + public long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { + return fsForPath(path).getLastModifiedTime(path, followSymlinks); + } + + @Override + public void setLastModifiedTime(PathFragment path, long newTime) throws IOException { + fsForPath(path).setLastModifiedTime(path, newTime); + } + + @Override + public FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + return fsForPath(path).stat(path, followSymlinks); + } + + @Override + public void createSymbolicLink( + PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType hint) + throws IOException { + fsForPath(linkPath).createSymbolicLink(linkPath, targetFragment, hint); + } + + @Override + public PathFragment readSymbolicLink(PathFragment path) throws IOException { + return fsForPath(path).readSymbolicLink(path); + } + + @Override + public boolean exists(PathFragment path, boolean followSymlinks) { + return fsForPath(path).exists(path, followSymlinks); + } + + @Override + public boolean exists(PathFragment path) { + return fsForPath(path).exists(path); + } + + @Override + public Collection getDirectoryEntries(PathFragment path) throws IOException { + return fsForPath(path).getDirectoryEntries(path); + } + + @Override + public boolean isReadable(PathFragment path) throws IOException { + return fsForPath(path).isReadable(path); + } + + @Override + public void setReadable(PathFragment path, boolean readable) throws IOException { + fsForPath(path).setReadable(path, readable); + } + + @Override + public boolean isWritable(PathFragment path) throws IOException { + return fsForPath(path).isWritable(path); + } + + @Override + public void setWritable(PathFragment path, boolean writable) throws IOException { + fsForPath(path).setWritable(path, writable); + } + + @Override + public boolean isExecutable(PathFragment path) throws IOException { + return fsForPath(path).isExecutable(path); + } + + @Override + public void setExecutable(PathFragment path, boolean executable) throws IOException { + fsForPath(path).setExecutable(path, executable); + } + + @Override + public InputStream getInputStream(PathFragment path) throws IOException { + return fsForPath(path).getInputStream(path); + } + + @Override + public SeekableByteChannel createReadWriteByteChannel(PathFragment path) throws IOException { + return fsForPath(path).createReadWriteByteChannel(path); + } + + @Override + public OutputStream getOutputStream(PathFragment path, boolean append, boolean internal) + throws IOException { + return fsForPath(path).getOutputStream(path, append, internal); + } + + @Override + public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IOException { + fsForPath(sourcePath).renameTo(sourcePath, targetPath); + } + + @Override + public void createFSDependentHardLink(PathFragment linkPath, PathFragment originalPath) + throws IOException { + fsForPath(originalPath).createFSDependentHardLink(linkPath, originalPath); + } + + @Override + public String getFileSystemType(PathFragment path) { + return fsForPath(path).getFileSystemType(path); + } + + @Override + public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) + throws IOException { + return fsForPath(path).getxattr(path, name, followSymlinks); + } + + @Override + public Path resolveSymbolicLinks(PathFragment path) throws IOException { + return fsForPath(path).resolveSymbolicLinks(path); + } + + @Nullable + @Override + public FileStatus statNullable(PathFragment path, boolean followSymlinks) { + return fsForPath(path).statNullable(path, followSymlinks); + } + + @Nullable + @Override + public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { + return fsForPath(path).statIfFound(path, followSymlinks); + } + + @Override + public boolean isFile(PathFragment path, boolean followSymlinks) { + return fsForPath(path).isFile(path, followSymlinks); + } + + @Override + public boolean isSpecialFile(PathFragment path, boolean followSymlinks) { + return fsForPath(path).isSpecialFile(path, followSymlinks); + } + + @Override + public boolean isSymbolicLink(PathFragment path) { + return fsForPath(path).isSymbolicLink(path); + } + + @Override + public boolean isDirectory(PathFragment path, boolean followSymlinks) { + return fsForPath(path).isDirectory(path, followSymlinks); + } + + @Override + public PathFragment readSymbolicLinkUnchecked(PathFragment path) throws IOException { + return fsForPath(path).readSymbolicLinkUnchecked(path); + } + + @Override + public Collection readdir(PathFragment path, boolean followSymlinks) throws IOException { + return fsForPath(path).readdir(path, followSymlinks); + } + + @Override + public void chmod(PathFragment path, int mode) throws IOException { + fsForPath(path).chmod(path, mode); + } + + + private final class RemoteExternalFileSystem + extends RemoteActionFileSystem.RemoteInMemoryFileSystem { + + RemoteExternalFileSystem(DigestHashFunction hashFunction) { + super(hashFunction); + } + + private RemoteActionExecutionContext makeRemoteContext(PathFragment relativePath) { + String repoName = relativePath.subFragment(0, 1).getBaseName(); + var metadata = + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, repoName, /* actionMetadata= */ null); + // Files in the remote external repo that Bazel reads are worth writing through to the + // disk cache, as they are likely to be read again on future cold builds. + return RemoteActionExecutionContext.create(metadata) + .withReadCachePolicy(RemoteActionExecutionContext.CachePolicy.ANY_CACHE) + .withWriteCachePolicy(RemoteActionExecutionContext.CachePolicy.ANY_CACHE); + } + + private FileArtifactValue getMetadata(PathFragment path) throws IOException { + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + return info.getMetadata(); + } + + @Override + public synchronized InputStream getInputStream(PathFragment path) throws IOException { + var relativePath = path.relativeTo(externalDirectory); + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + reporter.post( + new ExtendedEventHandler.FetchProgress() { + @Override + public String getResourceIdentifier() { + return relativePath.getPathString(); + } + + @Override + public String getProgress() { + return "(%s)".formatted(Utils.bytesCountToDisplayString(info.getSize())); + } + + @Override + public boolean isFinished() { + return false; + } + }); + var digest = DigestUtil.buildDigest(info.getMetadata().getDigest(), info.getSize()); + try { + var contentFuture = + cache.downloadBlob( + makeRemoteContext(relativePath), + path.getPathString(), + /* execPath= */ null, + digest); + waitForBulkTransfer(ImmutableList.of(contentFuture)); + return new ByteArrayInputStream(contentFuture.get()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new InterruptedIOException("interrupted while waiting for remote file transfer"); + } catch (BulkTransferException e) { + if (e.allCausedByCacheNotFoundException()) { + reposWithLostFiles.add(relativePath.getSegment(0)); + throw new DetailedIOException( + "%s/%s with digest %s is no longer available in the remote cache" + .formatted( + externalDirectory.getBaseName(), relativePath, DigestUtil.toString(digest)), + e, + FailureDetails.Filesystem.Code.REMOTE_FILE_EVICTED, + SkyFunctionException.Transience.TRANSIENT); + } + throw e; + } catch (ExecutionException e) { + throw new IllegalStateException("waitForBulkTransfer should have thrown", e); + } finally { + reporter.post( + new ExtendedEventHandler.FetchProgress() { + @Override + public String getResourceIdentifier() { + return relativePath.getPathString(); + } + + @Override + public String getProgress() { + return ""; + } + + @Override + public boolean isFinished() { + return true; + } + }); + } + } + + @Override + public byte[] getDigest(PathFragment path) throws IOException { + var info = + (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); + return info.getMetadata().getDigest(); + } + + @Override + public synchronized byte[] getFastDigest(PathFragment path) throws IOException { + return getDigest(path); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index d2565aebf0aeca..6fb1dd302d1926 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.buildtool.BuildRequest; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.ExecutionOptions; @@ -72,14 +73,17 @@ import com.google.devtools.build.lib.remote.logging.RemoteExecutionLog.LogEntry; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; +import com.google.devtools.build.lib.remote.options.RemoteStartupOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; +import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions; import com.google.devtools.build.lib.runtime.BlockWaitingModule; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.CommandLinePathFactory; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; import com.google.devtools.build.lib.runtime.ServerBuilder; @@ -99,6 +103,7 @@ import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingResult; @@ -130,7 +135,9 @@ public final class RemoteModule extends BlazeModule { MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); private final Set knownMissingCasDigests = Sets.newConcurrentHashSet(); + private boolean useRemoteRepoContentsCache; + @Nullable private PathFragment outputBase; @Nullable private AsynchronousMessageOutputStream rpcLogFile; @Nullable private ExecutorService executorService; @Nullable private RemoteActionContextProvider actionContextProvider; @@ -172,6 +179,28 @@ public ManagedChannel newChannel( private CredentialModule credentialModule; + @Override + public ImmutableList> getStartupOptions() { + return ImmutableList.of(RemoteStartupOptions.class); + } + + @Override + public void globalInit(OptionsParsingResult startupOptions) { + outputBase = startupOptions.getOptions(BlazeServerStartupOptions.class).outputBase; + useRemoteRepoContentsCache = + startupOptions.getOptions(RemoteStartupOptions.class).useRemoteRepoContentsCache; + } + + @Nullable + @Override + public FileSystem getFileSystemForBuildArtifacts(FileSystem nativeFs) { + if (!useRemoteRepoContentsCache) { + return null; + } + return new RemoteExternalOverlayFileSystem( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION), nativeFs); + } + @Override public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builder) { builder.addBuildEventArtifactUploaderFactory( @@ -264,6 +293,34 @@ private void initHttpAndDiskCache( remoteOutputChecker, outputService, knownMissingCasDigests); + actionInputFetcher = createActionInputFetcher(combinedCache); + } + + @Nullable + private RemoteActionInputFetcher createActionInputFetcher(@Nullable CombinedCache combinedCache) { + if (combinedCache == null) { + return null; + } + var coreOptions = env.getOptions().getOptions(CoreOptions.class); + var outputPermissions = + coreOptions != null && coreOptions.experimentalWritableOutputs + ? OutputPermissions.WRITABLE + : OutputPermissions.READONLY; + return new RemoteActionInputFetcher( + env.getReporter(), + env.getBuildRequestId(), + env.getCommandId().toString(), + combinedCache, + // The exec root is resolved lazily because it is not yet available in beforeCommand, when + // the remote repo contents cache sets up its input fetcher. + env::getExecRoot, + env.getOutputBase(), + tempPathGenerator, + remoteOutputChecker, + env.getOptions().getOptions(BuildRequestOptions.class) != null + ? env.getOutputDirectoryHelper() + : null, + outputPermissions); } @Override @@ -352,6 +409,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { FailureDetails.RemoteOptions.Code.DOWNLOADER_WITHOUT_GRPC_CACHE); } + tempPathGenerator = getTempPathGenerator(env); + if (!enableDiskCache && !enableHttpCache && !enableGrpcCache && !enableRemoteExecution) { // Quit if no remote caching or execution was enabled. actionContextProvider = @@ -699,14 +758,28 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { knownMissingCasDigests); } + actionInputFetcher = createActionInputFetcher(actionContextProvider.getCombinedCache()); + repositoryRemoteHelpersFactoryDelegate.init( - new RemoteRepositoryHelpersFactory( + new RepositoryRemoteHelpersFactoryImpl( actionContextProvider.getCombinedCache(), actionContextProvider.getRemoteExecutionClient(), buildRequestId, invocationId, remoteOptions.remoteInstanceName, - remoteOptions.remoteAcceptCached)); + remoteOptions.remoteAcceptCached, + remoteOptions.remoteUploadLocalResults)); + if (env.getDirectories().getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + remoteFs.beforeCommand( + actionContextProvider.getCombinedCache(), + actionInputFetcher, + env.getReporter(), + buildRequestId, + invocationId, + env.getSkyframeExecutor().getEvaluator()); + } + buildEventArtifactUploaderFactoryDelegate.init( new ByteStreamBuildEventArtifactUploaderFactory( executorService, @@ -903,6 +976,13 @@ public void afterCommand() { buildEventArtifactUploaderFactoryDelegate.reset(); repositoryRemoteHelpersFactoryDelegate.reset(); + // actionInputFetcher is non-null exactly when beforeCommand set up the + // RemoteExternalOverlayFileSystem, so only then must it be torn down here. + if (actionInputFetcher != null + && env.getDirectories().getOutputBase().getFileSystem() + instanceof RemoteExternalOverlayFileSystem remoteFs) { + remoteFs.afterCommand(); + } remoteDownloader = null; actionContextProvider = null; actionInputFetcher = null; @@ -971,7 +1051,12 @@ public void registerActionContexts( private TempPathGenerator getTempPathGenerator(CommandEnvironment env) throws AbruptExitException { - Path tempDir = env.getActionTempsDirectory().getChild("remote"); + // The temp directory is placed under the output base rather than the action temps directory + // (which lives under the exec root) because the exec root is not yet resolvable in + // beforeCommand, when the remote repo contents cache sets up its input fetcher. The output base + // is on the same file system as both the exec root and the external repo directory, so staged + // downloads can still be moved into place atomically. + Path tempDir = env.getOutputBase().getChild("remote_download_tmp"); if (tempDir.exists()) { env.getReporter() .handle(Event.warn("Found stale downloads from previous build, deleting...")); @@ -995,41 +1080,20 @@ private TempPathGenerator getTempPathGenerator(CommandEnvironment env) } @Override - public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) - throws AbruptExitException { - Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); - Preconditions.checkState(tempPathGenerator == null, "tempPathGenerator must be null"); + public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { Preconditions.checkNotNull(remoteOptions, "remoteOptions must not be null"); if (actionContextProvider == null) { return; } - tempPathGenerator = getTempPathGenerator(env); - actionContextProvider.setTempPathGenerator(tempPathGenerator); - CoreOptions coreOptions = env.getOptions().getOptions(CoreOptions.class); - OutputPermissions outputPermissions = - coreOptions.experimentalWritableOutputs - ? OutputPermissions.WRITABLE - : OutputPermissions.READONLY; - if (actionContextProvider.getCombinedCache() != null) { Preconditions.checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null"); Preconditions.checkNotNull(outputService, "remoteOutputService must not be null"); + Preconditions.checkNotNull(actionInputFetcher, "actionInputFetcher must not be null"); - actionInputFetcher = - new RemoteActionInputFetcher( - env.getReporter(), - env.getBuildRequestId(), - env.getCommandId().toString(), - actionContextProvider.getCombinedCache(), - env.getExecRoot(), - tempPathGenerator, - remoteOutputChecker, - env.getOutputDirectoryHelper(), - outputPermissions); env.getEventBus().register(actionInputFetcher); builder.setActionInputPrefetcher(actionInputFetcher); actionContextProvider.setActionInputFetcher(actionInputFetcher); @@ -1143,6 +1207,16 @@ public RepositoryRemoteExecutor createExecutor() { } return delegate.createExecutor(); } + + @Nullable + @Override + public RemoteRepoContentsCache createRepoContentsCache() { + RepositoryRemoteHelpersFactory delegate = this.delegate; + if (delegate == null) { + return null; + } + return delegate.createRepoContentsCache(); + } } @VisibleForTesting diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java new file mode 100644 index 00000000000000..2066cb9bb77909 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java @@ -0,0 +1,347 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.util.concurrent.Futures.immediateFuture; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; +import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; + +import build.bazel.remote.execution.v2.Action; +import build.bazel.remote.execution.v2.Command; +import build.bazel.remote.execution.v2.Directory; +import build.bazel.remote.execution.v2.Tree; +import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.exec.SpawnInputExpander; +import com.google.devtools.build.lib.exec.SpawnRunner; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy; +import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; +import com.google.devtools.build.lib.remote.common.RemotePathResolver; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; +import com.google.devtools.build.lib.unsafe.StringUnsafe; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.protobuf.ByteString; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.util.SortedMap; +import java.util.UUID; +import java.util.concurrent.ExecutionException; + +/** + * A cache for the contents of external repositories that is backed by an ordinary remote cache. + * + *

Upon a cache hit, the metadata of the files comprising the repository is downloaded and + * injected into a {@link RemoteExternalOverlayFileSystem}. Downloads of file contents only occur + * when Bazel needs to read a file (e.g., a BUILD or .bzl file) or if a file is an input to an + * action executed locally. This can save both time taken to execute repo rules and compute file + * digests and disk space required to store the contents of external repositories. + * + *

Repositories are cached as AC entries for a synthetic command with the predeclared input hash + * as the salt. The contents are represented as an output file for the marker file and an output + * directory for the contents. + * + *

At this point the cache only supports repository rules with no dependencies expressed at + * runtime. Verifying whether such dependencies are up to date can't be done via a single hash as + * the set of dependencies is not known ahead of time. Support for such rules would require a + * two-stage cache lookup in which the first lookup may produce multiple marker files. + */ +public final class RemoteRepoContentsCacheImpl implements RemoteRepoContentsCache { + private static final UUID GUID = UUID.fromString("f4a165a9-5557-45a7-bf25-230b6d42393a"); + private static final String MARKER_FILE_PATH = ".recorded_inputs"; + private static final String REPO_DIRECTORY_PATH = "repo_contents"; + + private static final Command COMMAND = + Command.newBuilder() + // A unique but nonsensical command that is valid on all platforms. It is never executed, + // but should pass all checks that an RE backend may apply to commands. + .addArguments(GUID.toString()) + .addOutputPaths(MARKER_FILE_PATH) + .addOutputPaths(REPO_DIRECTORY_PATH) + .addOutputFiles(MARKER_FILE_PATH) + .addOutputDirectories(REPO_DIRECTORY_PATH) + .build(); + private static final Directory INPUT_ROOT = Directory.getDefaultInstance(); + + private final CombinedCache cache; + private final String buildRequestId; + private final String commandId; + private final boolean acceptCached; + private final boolean uploadLocalResults; + private final DigestUtil digestUtil; + private final Action baseAction; + + public RemoteRepoContentsCacheImpl( + CombinedCache cache, + String buildRequestId, + String commandId, + boolean acceptCached, + boolean uploadLocalResults) { + this.buildRequestId = buildRequestId; + this.commandId = commandId; + this.cache = cache; + this.acceptCached = acceptCached; + this.uploadLocalResults = uploadLocalResults; + this.digestUtil = cache.digestUtil; + this.baseAction = + Action.newBuilder() + .setCommandDigest(digestUtil.compute(COMMAND)) + .setInputRootDigest(digestUtil.compute(INPUT_ROOT)) + .build(); + } + + @Override + public void addToCache( + RepositoryName repoName, + Path fetchedRepoDir, + Path fetchedRepoMarkerFile, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws InterruptedException { + // The repo contents cache only stores results in the remote cache when its file system overlay + // is installed, which only happens with --experimental_remote_repo_contents_cache. This matches + // the check in lookupCache and ensures a plain remote build does not pollute the action cache + // with repository entries. + if (!(fetchedRepoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem)) { + return; + } + var context = buildContext(repoName); + if (!context.getWriteCachePolicy().allowRemoteCache()) { + return; + } + try { + if (FileSystemUtils.readLinesAsLatin1(fetchedRepoMarkerFile).stream() + .filter(line -> !line.isEmpty()) + .count() + != 1) { + // This cache currently only supports marker files that contain nothing but the predeclared + // inputs hash. Repo rules with dependencies expressed only at runtime would require a + // two-stage cache lookup. Among the rules that are supported are http_archive and + // git_repository without patches. + return; + } + } catch (IOException e) { + reporter.handle( + Event.warn( + "Failed to read marker file repo %s, skipping: %s" + .formatted(repoName, e.getMessage()))); + } + var action = buildAction(predeclaredInputHash); + var actionKey = new ActionKey(digestUtil.compute(action)); + var remotePathResolver = new RepoRemotePathResolver(fetchedRepoMarkerFile, fetchedRepoDir); + try { + // TODO: Consider uploading asynchronously. + var unused = + UploadManifest.create( + /* remoteOptions= */ null, + cache.getRemoteCacheCapabilities(), + digestUtil, + remotePathResolver, + actionKey, + action, + COMMAND, + ImmutableList.of(fetchedRepoMarkerFile, fetchedRepoDir), + /* outErr= */ null, + /* exitCode= */ 0, + /* startTime= */ Instant.now(), + /* wallTimeInMs= */ 0, + /* preserveExecutableBit= */ true) + .upload(context, cache, reporter); + } catch (ExecException | IOException e) { + reporter.handle( + Event.warn( + "Failed to upload repo contents to remote cache for repo %s: %s" + .formatted(repoName, e.getMessage()))); + } + } + + @Override + public boolean lookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException { + if (!(repoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem remoteFs)) { + return false; + } + + var context = buildContext(repoName); + if (!context.getReadCachePolicy().allowRemoteCache()) { + return false; + } + var actionKey = new ActionKey(digestUtil.compute(buildAction(predeclaredInputHash))); + // The marker file is read right after and thus requested to be inlined. + var cachedActionResult = + cache.downloadActionResult( + context, actionKey, /* inlineOutErr= */ false, ImmutableSet.of(MARKER_FILE_PATH)); + if (cachedActionResult == null) { + return false; + } + + var actionResult = cachedActionResult.actionResult(); + if (actionResult.getExitCode() != 0 + || actionResult.getOutputFilesCount() != 1 + || actionResult.getOutputDirectoriesCount() != 1) { + reporter.handle( + Event.warn( + String.format( + "Unexpected action result for cached repo %s: exit code %d, %d output files, %d" + + " output directories", + repoName, + actionResult.getExitCode(), + actionResult.getOutputFilesCount(), + actionResult.getOutputDirectoriesCount()))); + return false; + } + + ListenableFuture markerFileContentFuture; + var markerFile = actionResult.getOutputFiles(0); + // Inlining is an optional feature, so we have to be prepared to download the marker file. + if (markerFile.getContents().isEmpty()) { + markerFileContentFuture = + cache.downloadBlob( + context, MARKER_FILE_PATH, /* execPath= */ null, markerFile.getDigest()); + } else { + markerFileContentFuture = immediateFuture(markerFile.getContents().toByteArray()); + } + var repoDirectory = actionResult.getOutputDirectories(0); + var repoDirectoryContentFuture = + Futures.transformAsync( + cache.downloadBlob( + context, REPO_DIRECTORY_PATH, /* execPath= */ null, repoDirectory.getTreeDigest()), + (treeBytes) -> immediateFuture(Tree.parseFrom(treeBytes)), + directExecutor()); + waitForBulkTransfer(ImmutableList.of(markerFileContentFuture, repoDirectoryContentFuture)); + String markerFileContent; + Tree repoDirectoryContent; + try { + markerFileContent = new String(markerFileContentFuture.get(), StandardCharsets.ISO_8859_1); + repoDirectoryContent = repoDirectoryContentFuture.get(); + } catch (ExecutionException e) { + throw new IllegalStateException("waitForBulkTransfer should have thrown", e); + } + var markerFileLines = + Splitter.on('\n') + .splitToStream(markerFileContent) + .filter(line -> !line.isEmpty()) + .collect(toImmutableList()); + if (markerFileLines.size() > 1) { + reporter.handle( + Event.warn( + "Marker file for repo %s has extra lines, skipping:\n%s" + .formatted( + repoName, + String.join("\n", markerFileLines.subList(1, markerFileLines.size()))))); + return false; + } + if (!markerFileLines.getFirst().equals(predeclaredInputHash)) { + reporter.handle( + Event.warn( + "Predeclared input hash mismatch for repo %s: expected %s, got %s" + .formatted(repoName, predeclaredInputHash, markerFileLines.getFirst()))); + return false; + } + + remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent); + return true; + } + + private RemoteActionExecutionContext buildContext(RepositoryName repoName) { + var metadata = + TracingMetadataUtils.buildMetadata( + buildRequestId, commandId, repoName.getName(), /* actionMetadata= */ null); + // Don't use the disk cache as `--repo_contents_cache` is a strictly better alternative for + // local caching. + return RemoteActionExecutionContext.create(metadata) + .withReadCachePolicy(acceptCached ? CachePolicy.REMOTE_CACHE_ONLY : CachePolicy.NO_CACHE) + .withWriteCachePolicy( + uploadLocalResults ? CachePolicy.REMOTE_CACHE_ONLY : CachePolicy.NO_CACHE); + } + + private Action buildAction(String predeclaredInputHash) { + // The predeclared input hash uniquely identifies the repo rule and all its attributes, but not + // dependencies established at runtime. We choose to embed it into the salt simply because that + // results in a constant Command message. + return baseAction.toBuilder() + .setSalt(ByteString.copyFrom(StringUnsafe.getByteArray(predeclaredInputHash))) + .build(); + } + + private record RepoRemotePathResolver(Path fetchedRepoMarkerFile, Path fetchedRepoDir) + implements RemotePathResolver { + + @Override + public String localPathToOutputPath(Path path) { + // Map repo marker file and contents to fixed locations under the fake remote exec root. + if (path.equals(fetchedRepoMarkerFile)) { + return MARKER_FILE_PATH; + } + if (path.equals(fetchedRepoDir)) { + return REPO_DIRECTORY_PATH; + } + return REPO_DIRECTORY_PATH + "/" + path.relativeTo(fetchedRepoDir).getPathString(); + } + + @Override + public String localPathToOutputPath(PathFragment execPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public String getWorkingDirectory() { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public Path outputPathToLocalPath(String outputPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public PathFragment localPathToExecPath(PathFragment localPath) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public SortedMap getInputMapping( + SpawnRunner.SpawnExecutionContext context, boolean willAccessRepeatedly) { + throw new UnsupportedOperationException("Not used"); + } + + @Override + public void walkInputs( + Spawn spawn, + SpawnRunner.SpawnExecutionContext context, + SpawnInputExpander.InputVisitor visitor) { + throw new UnsupportedOperationException("Not used"); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryHelpersFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java similarity index 74% rename from src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryHelpersFactory.java rename to src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java index 5de3969dc46d4e..5d3d95a022534f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryHelpersFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java @@ -14,12 +14,13 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteHelpersFactory; import javax.annotation.Nullable; -/** Factory for {@link RemoteRepositoryRemoteExecutor}. */ -class RemoteRepositoryHelpersFactory implements RepositoryRemoteHelpersFactory { +/** Factory for {@link RemoteRepositoryRemoteExecutor} and {@link RemoteRepoContentsCacheImpl}. */ +class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFactory { private final CombinedCache cache; @Nullable private final RemoteExecutionClient remoteExecutor; @@ -28,20 +29,23 @@ class RemoteRepositoryHelpersFactory implements RepositoryRemoteHelpersFactory { private final String remoteInstanceName; private final boolean acceptCached; + private final boolean uploadLocalResults; - RemoteRepositoryHelpersFactory( + RepositoryRemoteHelpersFactoryImpl( CombinedCache cache, @Nullable RemoteExecutionClient remoteExecutor, String buildRequestId, String commandId, String remoteInstanceName, - boolean acceptCached) { + boolean acceptCached, + boolean uploadLocalResults) { this.cache = cache; this.remoteExecutor = remoteExecutor; this.buildRequestId = buildRequestId; this.commandId = commandId; this.remoteInstanceName = remoteInstanceName; this.acceptCached = acceptCached; + this.uploadLocalResults = uploadLocalResults; } @Nullable @@ -59,4 +63,11 @@ public RepositoryRemoteExecutor createExecutor() { remoteInstanceName, acceptCached); } + + @Nullable + @Override + public RemoteRepoContentsCache createRepoContentsCache() { + return new RemoteRepoContentsCacheImpl( + cache, buildRequestId, commandId, acceptCached, uploadLocalResults); + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index 2d968895ed258b..65e12131f5a71f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -97,11 +97,12 @@ public class UploadManifest { private final RemotePathResolver remotePathResolver; private final ActionResult.Builder result; private final boolean allowAbsoluteSymlinks; + private final boolean preserveExecutableBit; private final ConcurrentHashMap digestToFile = new ConcurrentHashMap<>(); private final ConcurrentHashMap digestToBlobs = new ConcurrentHashMap<>(); @Nullable private ActionKey actionKey; - private Digest stderrDigest; - private Digest stdoutDigest; + private Digest stderrDigest = null; + private Digest stdoutDigest = null; public static UploadManifest create( RemoteOptions remoteOptions, @@ -112,10 +113,11 @@ public static UploadManifest create( Action action, Command command, Collection outputFiles, - FileOutErr outErr, + @Nullable FileOutErr outErr, int exitCode, Instant startTime, - int wallTimeInMs) + int wallTimeInMs, + boolean preserveExecutableBit) throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); result.setExitCode(exitCode); @@ -127,9 +129,12 @@ public static UploadManifest create( result, /* allowAbsoluteSymlinks= */ cacheCapabilities .getSymlinkAbsolutePathStrategy() - .equals(SymlinkAbsolutePathStrategy.Value.ALLOWED)); + .equals(SymlinkAbsolutePathStrategy.Value.ALLOWED), + preserveExecutableBit); manifest.addFiles(outputFiles); - manifest.setStdoutStderr(outErr); + if (outErr != null) { + manifest.setStdoutStderr(outErr); + } manifest.addAction(actionKey, action, command); if (manifest.getStderrDigest() != null) { result.setStderrDigest(manifest.getStderrDigest()); @@ -172,10 +177,25 @@ public UploadManifest( RemotePathResolver remotePathResolver, ActionResult.Builder result, boolean allowAbsoluteSymlinks) { + this( + digestUtil, + remotePathResolver, + result, + allowAbsoluteSymlinks, + /* preserveExecutableBit= */ false); + } + + public UploadManifest( + DigestUtil digestUtil, + RemotePathResolver remotePathResolver, + ActionResult.Builder result, + boolean allowAbsoluteSymlinks, + boolean preserveExecutableBit) { this.digestUtil = digestUtil; this.remotePathResolver = remotePathResolver; this.result = result; this.allowAbsoluteSymlinks = allowAbsoluteSymlinks; + this.preserveExecutableBit = preserveExecutableBit; } private void setStdoutStderr(FileOutErr outErr) throws IOException { @@ -229,7 +249,7 @@ void addFiles(Collection files) throws ExecException, IOException, Interru } if (statNoFollow.isFile() && !statNoFollow.isSpecialFile()) { Digest digest = digestUtil.compute(file, statNoFollow); - addFile(digest, file); + addFile(digest, file, statNoFollow); continue; } if (statNoFollow.isDirectory()) { @@ -251,7 +271,7 @@ void addFiles(Collection files) throws ExecException, IOException, Interru if (statFollow.isFile() && !statFollow.isSpecialFile()) { if (target.isAbsolute()) { // Symlink to file uploaded as a file. - addFile(digestUtil.compute(file, statFollow), file); + addFile(digestUtil.compute(file, statFollow), file, statNoFollow); } else { // Symlink to file uploaded as a symlink. if (target.isAbsolute()) { @@ -335,12 +355,12 @@ private void addDirectorySymbolicLink(Path file, PathFragment target) { result.addOutputSymlinks(outputSymlink); } - private void addFile(Digest digest, Path file) { + private void addFile(Digest digest, Path file, FileStatus statNoFollow) { result .addOutputFilesBuilder() .setPath(internalToUnicode(remotePathResolver.localPathToOutputPath(file))) .setDigest(digest) - .setIsExecutable(true); + .setIsExecutable(!preserveExecutableBit || (statNoFollow.getPermissions() & 0100) != 0); digestToFile.put(digest, file); } @@ -510,12 +530,13 @@ private void visitAsDirectory(Path path) { private void visitAsFile(Path path) throws IOException { Path parentPath = path.getParentDirectory(); + FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW); Digest digest = digestUtil.compute(path); FileNode node = FileNode.newBuilder() .setName(path.getBaseName()) .setDigest(digest) - .setIsExecutable(true) + .setIsExecutable(!preserveExecutableBit || (stat.getPermissions() & 0100) != 0) .build(); digestToFile.put(digest, path); dirToFiles.put(parentPath, node); diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java new file mode 100644 index 00000000000000..ef6c9558ecf19c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteStartupOptions.java @@ -0,0 +1,40 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.remote.options; + +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionsBase; + +/** + * Additional startup options provided by the {@link + * com.google.devtools.build.lib.remote.RemoteModule}. + */ +public final class RemoteStartupOptions extends OptionsBase { + @Option( + name = "experimental_remote_repo_contents_cache", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + """ + If enabled, the remote cache will be used to store the results of reproducible repository + rules. If a repository rule needs to be evaluated and its result is already in the remote + cache, the contents of the repository will be kept in an in-memory file system and are + only downloaded when needed, either by Bazel itself or an action that runs locally. + """) + public boolean useRemoteRepoContentsCache; +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index 6a56e5768b1f6a..468d5f86b78a92 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -83,6 +83,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; import java.util.function.BiFunction; import javax.annotation.Nullable; @@ -92,23 +93,22 @@ public final class Utils { private Utils() {} /** - * Returns the result of a {@link ListenableFuture} if successful, or throws any checked {@link - * Exception} directly if it's an {@link IOException} or else wraps it in an {@link IOException}. + * Returns the result of a {@link Future} if successful, or throws any checked {@link Exception} + * directly if it's an {@link IOException} or else wraps it in an {@link IOException}. * *

Cancel the future on {@link InterruptedException} */ - public static T getFromFuture(ListenableFuture f) - throws IOException, InterruptedException { + public static T getFromFuture(Future f) throws IOException, InterruptedException { return getFromFuture(f, /* cancelOnInterrupt */ true); } /** - * Returns the result of a {@link ListenableFuture} if successful, or throws any checked {@link - * Exception} directly if it's an {@link IOException} or else wraps it in an {@link IOException}. + * Returns the result of a {@link Future} if successful, or throws any checked {@link Exception} + * directly if it's an {@link IOException} or else wraps it in an {@link IOException}. * * @param cancelOnInterrupt cancel the future on {@link InterruptedException} if {@code true}. */ - public static T getFromFuture(ListenableFuture f, boolean cancelOnInterrupt) + public static T getFromFuture(Future f, boolean cancelOnInterrupt) throws IOException, InterruptedException { try { return f.get(); @@ -598,7 +598,7 @@ public static void waitForBulkTransfer(Iterable> t try { if (interruptedException == null) { // Wait for all transfers to finish. - getFromFuture(transfer, /* cancelOnInterrupt= */ true); + var unused = getFromFuture(transfer, /* cancelOnInterrupt= */ true); } else { transfer.cancel(true); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index fa6e16bc3571a1..453d06ec3ba748 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -401,6 +401,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/repository:external_package_exception", + "//src/main/java/com/google/devtools/build/lib:runtime/remote_repo_contents_cache", "//src/main/java/com/google/devtools/build/lib/repository:external_package_helper", "//src/main/java/com/google/devtools/build/lib/repository:external_rule_not_found_exception", "//src/main/java/com/google/devtools/build/lib/repository:repository_events", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index adf47f3bce3018..57e40d770200f8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.rules.repository.RepositoryFunction.AlreadyReportedRepositoryAccessException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.Reproducibility; +import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache; import com.google.devtools.build.lib.skyframe.AlreadyReportedException; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; @@ -112,6 +113,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction { private final Supplier> repoEnvironmentSupplier; private final Supplier> clientEnvironmentSupplier; private final LocalRepoContentsCache repoContentsCache; + @Nullable private RemoteRepoContentsCache remoteRepoContentsCache; public RepositoryDelegatorFunction( ImmutableMap handlers, @@ -132,6 +134,10 @@ public RepositoryDelegatorFunction( this.repoContentsCache = repoContentsCache; } + public void setRemoteRepoContentsCache(RemoteRepoContentsCache remoteRepoContentsCache) { + this.remoteRepoContentsCache = remoteRepoContentsCache; + } + @Nullable @Override public SkyValue compute(SkyKey skyKey, Environment env) @@ -257,6 +263,27 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } } + + if (remoteRepoContentsCache != null) { + try { + if (remoteRepoContentsCache.lookupCache( + repositoryName, repoRoot, digestWriter.predeclaredInputHash, env.getListener())) { + env.getListener() + .handle( + Event.debug( + "Got %s from the remote repo contents cache".formatted(repositoryName))); + return new RepositoryDirectoryValue.Success( + repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring); + } + } catch (IOException e) { + throw new RepositoryFunctionException( + new IOException( + "error looking up repo %s in remote repo contents cache: %s" + .formatted(repositoryName, e.getMessage()), + e), + Transience.TRANSIENT); + } + } } /* At this point: This is a force fetch, a local repository, OR The repository cache is old or @@ -274,31 +301,40 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } digestWriter.writeMarkerFile(result.recordedInputValues()); - if (repoContentsCache.isEnabled() - && result.reproducible() == Reproducibility.YES - && !handler.isLocal(rule)) { - // This repo is eligible for the repo contents cache. - Path cachedRepoDir; - try { - cachedRepoDir = - repoContentsCache.moveToCache( - repoRoot, digestWriter.markerPath, digestWriter.predeclaredInputHash); - } catch (IOException e) { - throw new RepositoryFunctionException( - new IOException( - "error moving repo @@%s into the repo contents cache: %s" - .formatted(rule.getName(), e.getMessage()), - e), - Transience.TRANSIENT); + if (result.reproducible() == Reproducibility.YES && !handler.isLocal(rule)) { + if (repoContentsCache.isEnabled()) { + // This repo is eligible for the repo contents cache. + Path cachedRepoDir; + try { + cachedRepoDir = + repoContentsCache.moveToCache( + repoRoot, digestWriter.markerPath, digestWriter.predeclaredInputHash); + } catch (IOException e) { + throw new RepositoryFunctionException( + new IOException( + "error moving repo @@%s into the repo contents cache: %s" + .formatted(rule.getName(), e.getMessage()), + e), + Transience.TRANSIENT); + } + // Don't forget to register a FileValue on the cache repo dir, so that we know to + // refetch + // if the cache entry gets GC'd from under us. + if (env.getValue( + FileValue.key( + RootedPath.toRootedPath( + Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir))) + == null) { + return null; + } } - // Don't forget to register a FileValue on the cache repo dir, so that we know to refetch - // if the cache entry gets GC'd from under us. - if (env.getValue( - FileValue.key( - RootedPath.toRootedPath( - Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir))) - == null) { - return null; + if (remoteRepoContentsCache != null) { + remoteRepoContentsCache.addToCache( + repositoryName, + repoRoot, + digestWriter.markerPath, + digestWriter.predeclaredInputHash, + env.getListener()); } } return new RepositoryDirectoryValue.Success( diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java new file mode 100644 index 00000000000000..8c4f0f25d1cf41 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/runtime/RemoteRepoContentsCache.java @@ -0,0 +1,45 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.runtime; + +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.vfs.Path; +import java.io.IOException; + +/** A remote cache for the contents of external repositories. */ +public interface RemoteRepoContentsCache { + /** Adds a repository that has been fetched locally to the remote cache. */ + void addToCache( + RepositoryName repoName, + Path fetchedRepoDir, + Path fetchedRepoMarkerFile, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws InterruptedException; + + /** + * Retrieves a repository from the remote cache if possible. + * + * @return true if there was a cache hit and the repository has been fetched into the given + * directory. + */ + boolean lookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException; +} diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java b/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java index fa4d138bff34ca..d3cd1e66ced683 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/RepositoryRemoteHelpersFactory.java @@ -15,10 +15,14 @@ import javax.annotation.Nullable; -/** Factory for {@link RepositoryRemoteExecutor}. */ +/** Factory for {@link RepositoryRemoteExecutor} and {@link RemoteRepoContentsCache}. */ public interface RepositoryRemoteHelpersFactory { /** Returns a new {@link RepositoryRemoteExecutor} or {@code null}. */ @Nullable RepositoryRemoteExecutor createExecutor(); + + /** Returns a new {@link RemoteRepoContentsCache} or {@code null}. */ + @Nullable + RemoteRepoContentsCache createRepoContentsCache(); } 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 77fedd0f818fa6..c1fcd687ca2ca3 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 @@ -132,6 +132,18 @@ private ActionOutputMetadataStore( this.execRoot = checkNotNull(execRoot); } + /** + * Relativizes the given path against the exec root, falling back to the output base for paths + * not under the exec root (e.g., external repos on 8.7.0 which are at output_base/external/). + */ + private PathFragment relativizeToExecRootOrOutputBase(PathFragment path) { + if (path.startsWith(execRoot)) { + return path.relativeTo(execRoot); + } + // External repos on 8.7.0 are at output_base/external/, a sibling of execroot/. + return path.relativeTo(execRoot.getParentDirectory().getParentDirectory()); + } + private void putArtifactData(Artifact artifact, FileArtifactValue value) { Preconditions.checkArgument( !artifact.isTreeArtifact() && !artifact.isChildOfDeclaredDirectory(), @@ -326,7 +338,8 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa tree.setMaterializationExecPath( metadata .getMaterializationExecPath() - .orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot))); + .orElse(relativizeToExecRootOrOutputBase( + treeDir.resolveSymbolicLinks().asFragment()))); } return tree.build(); @@ -469,7 +482,7 @@ private FileArtifactValue constructFileArtifactValue( value = RemoteFileArtifactValue.createFromExistingWithMaterializationPath( (RemoteFileArtifactValue) value, - statAndValue.realPath().asFragment().relativeTo(execRoot)); + relativizeToExecRootOrOutputBase(statAndValue.realPath().asFragment())); } // Ensure that we don't have both an injected digest and a digest from the filesystem. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index 1158ed7a71e3c3..619860b2716293 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithMetadata; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot; import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode; @@ -313,6 +314,12 @@ private SkyValue createSourceValue(Artifact artifact, Environment env) if (!fileValue.exists()) { return new MissingArtifactValue(artifact); } + if (fileValue.realFileStateValue() + instanceof RegularFileStateValueWithMetadata valueWithMetadata) { + var metadata = valueWithMetadata.getMetadata(); + sourceArtifactsSeen.accumulate(metadata); + return metadata; + } if (fileValue.isDirectory()) { env.getListener().post(SourceDirectoryEvent.create(artifact.getExecPath())); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java index 699753c56680b6..0cad4b05a01c41 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java @@ -17,10 +17,12 @@ import com.google.devtools.build.lib.io.InconsistentFilesystemException; import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.FileType; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; +import com.google.devtools.build.lib.vfs.DetailedIOException; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; import java.util.function.Supplier; @@ -70,9 +72,11 @@ public FileStateValue compute(SkyKey skyKey, Environment env) } catch (ExternalFilesHelper.NonexistentImmutableExternalFileException e) { return FileStateValue.NONEXISTENT_FILE_STATE_NODE; } catch (InconsistentFilesystemException e) { - throw new FileStateFunctionException(e); + throw new FileStateFunctionException(e, Transience.TRANSIENT); + } catch (DetailedIOException e) { + throw new FileStateFunctionException(e, e.getTransience()); } catch (IOException e) { - throw new FileStateFunctionException(e); + throw new FileStateFunctionException(e, Transience.TRANSIENT); } } @@ -83,13 +87,13 @@ public FileStateValue compute(SkyKey skyKey, Environment env) public static final class FileStateFunctionException extends SkyFunctionException { private final boolean isCatastrophic; - private FileStateFunctionException(InconsistentFilesystemException e) { - super(e, Transience.TRANSIENT); + private FileStateFunctionException(InconsistentFilesystemException e, Transience transience) { + super(e, transience); this.isCatastrophic = true; } - private FileStateFunctionException(IOException e) { - super(e, Transience.TRANSIENT); + private FileStateFunctionException(IOException e, Transience transience) { + super(e, transience); this.isCatastrophic = false; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 06e78fc7f8169b..8f3491eb5a56c2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -56,6 +56,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.server.FailureDetails.Filesystem; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; import com.google.devtools.build.lib.skyframe.IgnoredSubdirectoriesValue.InvalidIgnorePathException; @@ -65,6 +66,7 @@ import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.DetailedIOException; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -1217,7 +1219,7 @@ private CompiledBuildFile compileBuildFile( // read BUILD file Path inputFile = buildFilePath.asPath(); - byte[] buildFileBytes = null; + byte[] buildFileBytes; try { buildFileBytes = buildFileValue.isSpecialFile() @@ -1230,14 +1232,24 @@ private CompiledBuildFile compileBuildFile( if (buildFileBytes == null) { // Note that we did the work that led to this IOException, so we should // conservatively report this error as transient. - throw PackageFunctionException.builder() - .setType(PackageFunctionException.Type.BUILD_FILE_CONTAINS_ERRORS) - .setTransience(Transience.TRANSIENT) - .setPackageIdentifier(packageId) - .setMessage(e.getMessage()) - .setException(e) - .setPackageLoadingCode(PackageLoading.Code.BUILD_FILE_MISSING) - .build(); + var builder = + PackageFunctionException.builder() + .setType(PackageFunctionException.Type.BUILD_FILE_CONTAINS_ERRORS) + .setTransience(Transience.TRANSIENT) + .setPackageIdentifier(packageId) + .setMessage(e.getMessage()) + .setException(e); + // A DetailedIOException is only thrown for remote cache evictions of repo contents, whose + // filesystem failure detail (and the associated exit code) must take precedence over the + // generic package loading code so that the build can be retried. + if (e instanceof DetailedIOException detailedIoException + && detailedIoException.getDetailedExitCode().getFailureDetail().hasFilesystem()) { + builder.setFilesystemCode( + detailedIoException.getDetailedExitCode().getFailureDetail().getFilesystem().getCode()); + } else { + builder.setPackageLoadingCode(PackageLoading.Code.BUILD_FILE_MISSING); + } + throw builder.build(); } // If control flow reaches here, we're in territory that is deliberately unsound. // See the javadoc for ActionOnIOExceptionReadingBuildFile. @@ -1584,6 +1596,7 @@ static class Builder { private Exception exception; private String message; private PackageLoading.Code packageLoadingCode; + private Filesystem.Code filesystemCode; @CanIgnoreReturnValue Builder setType(Type exceptionType) { @@ -1621,6 +1634,12 @@ Builder setPackageLoadingCode(PackageLoading.Code packageLoadingCode) { return this; } + @CanIgnoreReturnValue + Builder setFilesystemCode(Filesystem.Code filesystemCode) { + this.filesystemCode = filesystemCode; + return this; + } + @Override public int hashCode() { return Objects.hash( @@ -1645,8 +1664,16 @@ public boolean equals(Object other) { NoSuchPackageException buildCause() { checkNotNull(exceptionType, "The NoSuchPackageException type must be set."); - checkNotNull(packageLoadingCode, "The PackageLoading code must be set."); - DetailedExitCode detailedExitCode = createDetailedExitCode(message, packageLoadingCode); + DetailedExitCode detailedExitCode; + if (filesystemCode != null) { + detailedExitCode = createDetailedExitCodeWithFilesystemCode(message, filesystemCode); + } else { + checkNotNull( + packageLoadingCode, + "Either the Filesystem code or the PackageLoading code must be set."); + detailedExitCode = + createDetailedExitCodeWithPackageLoadingCode(message, packageLoadingCode); + } return exceptionType.create(packageIdentifier, message, detailedExitCode, exception); } @@ -1655,7 +1682,7 @@ PackageFunctionException build() { buildCause(), checkNotNull(transience, "Transience must be set")); } - private static DetailedExitCode createDetailedExitCode( + private static DetailedExitCode createDetailedExitCodeWithPackageLoadingCode( String message, PackageLoading.Code packageLoadingCode) { return DetailedExitCode.of( FailureDetail.newBuilder() @@ -1663,6 +1690,15 @@ private static DetailedExitCode createDetailedExitCode( .setPackageLoading(PackageLoading.newBuilder().setCode(packageLoadingCode).build()) .build()); } + + private static DetailedExitCode createDetailedExitCodeWithFilesystemCode( + String message, Filesystem.Code filesystemCode) { + return DetailedExitCode.of( + FailureDetail.newBuilder() + .setMessage(message) + .setFilesystem(Filesystem.newBuilder().setCode(filesystemCode)) + .build()); + } } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java index 6abd92414d4adc..75412a2991e655 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns; import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey; import com.google.devtools.build.lib.util.DetailedExitCode; +import com.google.devtools.build.lib.vfs.DetailedIOException; import com.google.devtools.build.skyframe.ErrorInfo; import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyKey; @@ -172,6 +173,10 @@ private static TargetParsingException wrapException( exception, ((NoSuchPackageException) exception).getDetailedExitCode()); } + if (exception instanceof DetailedIOException detailedException) { + return new TargetParsingException( + detailedException.getMessage(), exception, detailedException.getDetailedExitCode()); + } BugReport.sendNonFatalBugReport( new IllegalStateException("Unexpected exception: " + debugging, exception)); String message = "Target parsing failed due to unexpected exception: " + exception.getMessage(); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD index d6a0dbbfada769..e1cf62d955f910 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD +++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD @@ -71,13 +71,16 @@ java_library( "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:filetype", "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/util:string_encoding", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/com/google/devtools/common/options", + "//src/main/protobuf:failure_details_java_proto", "//third_party:caffeine", "//third_party:error_prone_annotations", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/vfs/DetailedIOException.java b/src/main/java/com/google/devtools/build/lib/vfs/DetailedIOException.java new file mode 100644 index 00000000000000..b00211290abd4f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/vfs/DetailedIOException.java @@ -0,0 +1,53 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.vfs; + +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.server.FailureDetails.Filesystem; +import com.google.devtools.build.lib.skyframe.DetailedException; +import com.google.devtools.build.lib.util.DetailedExitCode; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import java.io.IOException; + +/** + * An {@link IOException} that includes {@link DetailedExitCode} and {@link Transience}. Currently + * only used for {@link Filesystem} exceptions. + */ +public final class DetailedIOException extends IOException implements DetailedException { + + private final DetailedExitCode detailedExitCode; + private final Transience transience; + + public DetailedIOException( + String message, IOException cause, Filesystem.Code filesystemCode, Transience transience) { + super(message, cause); + this.detailedExitCode = + DetailedExitCode.of( + FailureDetail.newBuilder() + .setMessage(message) + .setFilesystem(Filesystem.newBuilder().setCode(filesystemCode)) + .build()); + this.transience = transience; + } + + @Override + public DetailedExitCode getDetailedExitCode() { + return detailedExitCode; + } + + public Transience getTransience() { + return transience; + } +} diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 5691ff1dbd541c..f1001b715a9f18 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -544,6 +544,7 @@ message Filesystem { [(metadata) = { exit_code: 2 }]; FILESYSTEM_JNI_NOT_AVAILABLE = 8 [(metadata) = { exit_code: 36 }]; FAILED_TO_LOCK_INSTALL_BASE = 12 [(metadata) = { exit_code: 36 }]; + REMOTE_FILE_EVICTED = 13 [(metadata) = { exit_code: 39 }]; reserved 7, 9, 10; // For internal use } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 5499ab13bab100..7efb13e90952e8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -84,6 +84,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_metadata", "//src/main/java/com/google/devtools/build/lib/actions:runfiles_tree", + "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", @@ -149,6 +150,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/exec/util", "//src/test/java/com/google/devtools/build/lib/remote/util", "//src/test/java/com/google/devtools/build/lib/testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:auth", "//third_party:caffeine", diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index dba0c00671e76b..49cd10aec1e20c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -701,7 +701,8 @@ private ActionResult upload( outErr, /* exitCode= */ 0, /* startTime= */ null, - /* wallTimeInMs= */ 0); + /* wallTimeInMs= */ 0, + /* preserveExecutableBit= */ false); return uploadManifest.upload(context, combinedCache, NullEventHandler.INSTANCE); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index c6ca2ef8f0b939..d240f4d12d8477 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -77,7 +77,8 @@ protected AbstractActionInputPrefetcher createPrefetcher(Map c "none", "none", combinedCache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, DUMMY_REMOTE_OUTPUT_CHECKER, ActionOutputDirectoryHelper.createForTesting(), @@ -94,7 +95,8 @@ public void testStagingVirtualActionInput() throws Exception { "none", "none", combinedCache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, DUMMY_REMOTE_OUTPUT_CHECKER, ActionOutputDirectoryHelper.createForTesting(), @@ -128,7 +130,8 @@ public void testStagingEmptyVirtualActionInput() throws Exception { "none", "none", combinedCache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, DUMMY_REMOTE_OUTPUT_CHECKER, ActionOutputDirectoryHelper.createForTesting(), diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index e17ea1efcaa0f8..0320938631b843 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -2677,7 +2677,8 @@ private FakeSpawnExecutionContext newSpawnExecutionContext(Spawn spawn, FileOutE "none", "none", cache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, remoteOutputChecker, ActionOutputDirectoryHelper.createForTesting(), diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index e3ae3b4e28868a..440dc088264be8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java @@ -34,6 +34,7 @@ import com.google.common.eventbus.EventBus; import com.google.common.truth.extensions.proto.ProtoTruth; import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; @@ -49,6 +50,7 @@ import com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollectorIdleTask; import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader; import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions; import com.google.devtools.build.lib.runtime.BlazeWorkspace; @@ -61,6 +63,7 @@ import com.google.devtools.build.lib.runtime.commands.BuildCommand; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -167,6 +170,13 @@ private static CommandEnvironment createTestCommandEnvironment( .addBlazeModule(new CredentialModule()) .addBlazeModule(remoteModule) .addBlazeModule(new BlockWaitingModule()) + .addBlazeModule( + new BlazeModule() { + @Override + public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) { + builder.setRunfilesPrefix(TestConstants.WORKSPACE_NAME); + } + }) .build(); BlazeDirectories directories = diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 4c73ac618fd116..1fb1facce5ecb9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -273,7 +273,8 @@ private FakeSpawnExecutionContext getSpawnContext(Spawn spawn) { "none", "none", cache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, remoteOutputChecker, ActionOutputDirectoryHelper.createForTesting(), diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index c74e214be934f3..8bc7afd437a711 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -1569,7 +1569,8 @@ private FakeSpawnExecutionContext getSpawnContext(Spawn spawn) { "none", "none", remoteCache, - execRoot, + () -> execRoot, + execRoot.getParentDirectory(), tempPathGenerator, remoteOutputChecker, ActionOutputDirectoryHelper.createForTesting(), diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java index 0d03dbe9dc405e..62afde6b98a8d0 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java @@ -760,4 +760,102 @@ private Path createDirectoryWithSymlinkToSpecialFile( return dir; } + + @Test + public void actionResult_preserveExecutableBit_executableFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path file = execRoot.getRelative("file"); + FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); + file.setExecutable(true); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(file)); + Digest digest = digestUtil.compute(file); + assertThat(um.getDigestToFile()).containsExactly(digest, file); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("file").setDigest(digest).setIsExecutable(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_preserveExecutableBit_nonExecutableFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path file = execRoot.getRelative("file"); + FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); + file.setExecutable(false); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(file)); + Digest digest = digestUtil.compute(file); + assertThat(um.getDigestToFile()).containsExactly(digest, file); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("file").setDigest(digest).setIsExecutable(false); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_preserveExecutableBit_mixedFilesInDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path executableFile = execRoot.getRelative("dir/executable"); + FileSystemUtils.writeContent(executableFile, new byte[] {1, 2, 3}); + executableFile.setExecutable(true); + Path nonExecutableFile = execRoot.getRelative("dir/nonexecutable"); + FileSystemUtils.writeContent(nonExecutableFile, new byte[] {4, 5, 6}); + nonExecutableFile.setExecutable(false); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /* allowAbsoluteSymlinks= */ false, + /* preserveExecutableBit= */ true); + um.addFiles(ImmutableList.of(dir)); + + Digest executableDigest = digestUtil.compute(executableFile); + Digest nonExecutableDigest = digestUtil.compute(nonExecutableFile); + assertThat(um.getDigestToFile()) + .containsExactly(executableDigest, executableFile, nonExecutableDigest, nonExecutableFile); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("executable") + .setDigest(executableDigest) + .setIsExecutable(true)) + .addFiles( + FileNode.newBuilder() + .setName("nonexecutable") + .setDigest(nonExecutableDigest) + .setIsExecutable(false))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } } diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD index 4d7e56996f5c9b..60991b26682fa1 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -422,6 +422,18 @@ py_test( ], ) +py_test( + name = "remote_repo_contents_cache_test", + size = "large", + srcs = ["bzlmod/remote_repo_contents_cache_test.py"], + shard_count = 2, + tags = ["requires-network"], + deps = [ + ":bzlmod_test_utils", + ":test_base", + ], +) + py_test( name = "bzlmod_credentials_test", size = "large", diff --git a/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py new file mode 100644 index 00000000000000..57a941e824fa9b --- /dev/null +++ b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py @@ -0,0 +1,622 @@ +# pylint: disable=g-backslash-continuation +# Copyright 2025 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# pylint: disable=g-long-ternary +# pylint: disable=g-bad-todo + +import json +import os +import re +from absl.testing import absltest +from src.test.py.bazel import test_base + + +class RemoteRepoContentsCacheTest(test_base.TestBase): + + def setUp(self): + test_base.TestBase.setUp(self) + worker_port = self.StartRemoteWorker() + self.ScratchFile( + '.bazelrc', + [ + 'startup --experimental_remote_repo_contents_cache', + # Only use the remote repo contents cache. + 'common --repo_contents_cache=', + 'common --remote_cache=grpc://localhost:' + str(worker_port), + 'common --auth_enabled=false', + 'common --remote_timeout=3600s', + 'common --verbose_failures', + ], + ) + + def tearDown(self): + test_base.TestBase.tearDown(self) + self.StopRemoteWorker() + + def RepoDir(self, repo_name, cwd=None): + _, stdout, _ = self.RunBazel(['info', 'output_base'], cwd=cwd) + self.assertLen(stdout, 1) + output_base = stdout[0].strip() + + _, stdout, _ = self.RunBazel(['mod', 'dump_repo_mapping', ''], cwd=cwd) + self.assertLen(stdout, 1) + mapping = json.loads(stdout[0]) + canonical_repo_name = mapping[repo_name] + + return output_base + '/external/' + canonical_repo_name + + def testCachedAfterCleanExpunge(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging, without using repo contents cache: not cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel([ + '--noexperimental_remote_repo_contents_cache', + 'build', + '@my_repo//:haha', + ]) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenPredeclaredInputsChange(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 1)', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, attrs={"data":attr.int()})', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change predeclared inputs: not cached + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 2)', + ], + ) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous predeclared inputs: cached (even after expunging) + self.RunBazel(['clean', '--expunge']) + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo", data = 1)', + ], + ) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenRecordedInputsChange_dynamicDep(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.watch(Label("@//:data.txt"))', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + self.ScratchFile('data.txt', ['one']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change recorded inputs: not cached + self.ScratchFile('data.txt', ['two']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous recorded inputs: not cached + # TODO: This is the current behavior, but it's not desired. Support for + # caching repos with dynamic deps should be added. + self.RunBazel(['clean', '--expunge']) + self.ScratchFile('data.txt', ['one']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNotCachedWhenRecordedInputsChange_staticDep(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.os.environ.get("LOLOL")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, environ = ["LOLOL"])', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'lol'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change recorded inputs: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'kek'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Change back to previous recorded inputs: cached (even after expunging) + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], env_add={'LOLOL': 'lol'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + def testNoThrashingBetweenWorkspaces(self): + module_bazel_lines = [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ] + repo_bzl_lines = [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' rctx.os.environ.get("LOLOL")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl, environ = ["LOLOL"])', + ] + # Set up two workspaces with exactly the same repo definition (same + # predeclared inputs) + dir_a = self.ScratchDir('a') + dir_b = self.ScratchDir('b') + self.ScratchFile('a/MODULE.bazel', module_bazel_lines) + self.ScratchFile('b/MODULE.bazel', module_bazel_lines) + self.ScratchFile('a/BUILD.bazel') + self.ScratchFile('b/BUILD.bazel') + self.ScratchFile('a/repo.bzl', repo_bzl_lines) + self.ScratchFile('b/repo.bzl', repo_bzl_lines) + self.CopyFile(self.Path('.bazelrc'), 'a/.bazelrc') + self.CopyFile(self.Path('.bazelrc'), 'b/.bazelrc') + + repo_dir_a = self.RepoDir('my_repo', cwd=dir_a) + repo_dir_b = self.RepoDir('my_repo', cwd=dir_b) + + # First fetch in A: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_a, env_add={'LOLOL': 'lol'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir_a, 'BUILD'))) + + # Fetch in B (with same env variable): cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_b, env_add={'LOLOL': 'lol'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir_b, 'BUILD'))) + + # Change env variable: not cached + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_b, env_add={'LOLOL': 'rofl'} + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir_b, 'BUILD'))) + + # Building A again even after expunging: cached + self.RunBazel(['clean', '--expunge'], cwd=dir_a) + _, _, stderr = self.RunBazel( + ['build', '@my_repo//:haha'], cwd=dir_a, env_add={'LOLOL': 'rofl'} + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir_a, 'BUILD'))) + + def testAccessFromOtherRepo_read(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + 'other_repo = use_repo_rule("//:other_repo.bzl", "other_repo")', + 'other_repo(name = "other", build_file = "@my_repo//:BUILD")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'other_repo.bzl', + [ + 'def _other_repo_impl(rctx):', + ' rctx.file("BUILD", rctx.read(rctx.path(rctx.attr.build_file)))', + ' return rctx.repo_metadata()', + ( + 'other_repo = repository_rule(_other_repo_impl,' + ' attrs={"build_file": attr.label()})' + ), + ], + ) + + repo_dir = self.RepoDir('my_repo') + other_repo_dir = self.RepoDir('other') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: fetch my_repo only, not materialized + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Fetch other: my_repo materialized + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + # Materialized repo is not refetched after a shutdown + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + def testAccessFromOtherRepo_symlink(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + 'other_repo = use_repo_rule("//:other_repo.bzl", "other_repo")', + 'other_repo(name = "other", build_file = "@my_repo//:BUILD")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'other_repo.bzl', + [ + 'def _other_repo_impl(rctx):', + ' rctx.symlink(rctx.path(rctx.attr.build_file), "BUILD")', + ' return rctx.repo_metadata()', + ( + 'other_repo = repository_rule(_other_repo_impl,' + ' attrs={"build_file": attr.label()})' + ), + ], + ) + + repo_dir = self.RepoDir('my_repo') + other_repo_dir = self.RepoDir('other') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # After expunging: fetch my_repo only, not materialized + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + + # Fetch other: my_repo materialized + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + # Materialized repo is not refetched after a shutdown + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', '@other//:haha']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD'))) + + def testUseRepoFileInBuildRule_actionUsesCache(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "exports_files([\'data.txt\'])")', + ' rctx.file("data.txt", "hello")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'main/BUILD.bazel', + [ + 'genrule(', + ' name = "use_data",', + ' srcs = ["@my_repo//:data.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $< > $@",', + ')', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + # After expunging: repo and build action cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD", "exports_files([\'data.txt\'])")', + ' rctx.file("data.txt", "hello")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + self.ScratchFile( + 'main/BUILD.bazel', + [ + 'genrule(', + ' name = "use_data",', + ' srcs = ["@my_repo//:data.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $< > $@",', + ' tags = ["no-cache"],', + ')', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + # After expunging: repo and build action cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '//main:use_data']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt'))) + self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt'))) + with open(self.Path('bazel-bin/main/out.txt')) as f: + self.assertEqual(f.read(), 'hello') + + def testLostRemoteFile_build(self): + # Create a repo with two BUILD files (one in a subpackage), build a target + # from one to cause it to be cached, then build that target again after + # expunging to verify it is cached. + # Then, restart the worker and build a target in the other build file. + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ( + ' rctx.file("BUILD", "filegroup(name=\'root\',' + " srcs=['root.txt'])\")" + ), + ' rctx.file("root.txt", "root")', + ( + ' rctx.file("sub/BUILD", "filegroup(name=\'sub\',' + " srcs=['sub.txt'])\")" + ), + ' rctx.file("sub/sub.txt", "sub")', + ' print("JUST FETCHED")', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(_repo_impl)', + ], + ) + + repo_dir = self.RepoDir('my_repo') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@my_repo//:root']) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # After expunging: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//:root']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # Lose all remote files. + self.ClearRemoteCache() + + # Build the other target: fails due to the lost input + # TODO: #26450 - Assert success and enable the checks below. + _, _, stderr = self.RunBazel( + ['build', '@my_repo//sub:sub'], allow_failure=True + ) + self.assertEqual( + 1, + stderr.count( + 'Found transient remote cache error, retrying the build...' + ), + ) + canonical_repo_name = repo_dir[repo_dir.rfind('/') + 1 :] + stderr = '\n'.join(stderr) + self.assertRegex( + stderr, + 'external/%s/sub/BUILD with digest .*/.* no longer available in the' + ' remote cache' + % re.escape(canonical_repo_name), + ) + self.assertIn('JUST FETCHED', stderr) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + # After expunging again: cached + self.RunBazel(['clean', '--expunge']) + _, _, stderr = self.RunBazel(['build', '@my_repo//sub:sub']) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'root.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'sub/sub.txt'))) + + +if __name__ == '__main__': + absltest.main() diff --git a/src/test/py/bazel/test_base.py b/src/test/py/bazel/test_base.py index 35f4c68c1ead07..eeee99fa3e41a1 100644 --- a/src/test/py/bazel/test_base.py +++ b/src/test/py/bazel/test_base.py @@ -402,7 +402,7 @@ def StartRemoteWorker(self): # worker path must be as short as possible so we don't exceed Windows # path length limits, so we run straight in TEMP. This should ideally # be set to something like C:\temp. On CI this is set to D:\temp. - worker_path = TestBase.GetEnv('TEMP') + worker_path = tempfile.mkdtemp(dir=TestBase.GetEnv('TEMP')) worker_exe = self.Rlocation('io_bazel/src/tools/remote/worker.exe') else: worker_path = tempfile.mkdtemp(dir=self._tests_root) @@ -464,6 +464,13 @@ def StopRemoteWorker(self): shutil.rmtree(self._cas_path) + def ClearRemoteCache(self): + """Clears the CAS of the "local remote worker".""" + self.assertIsNotNone(self._cas_path) + shutil.rmtree(self._cas_path) + # The worker needs the CAS path as well as the tmp dir to exist. + os.makedirs(os.path.join(self._cas_path, 'tmp')) + def RunProgram( self, args, diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 4acba053b0d244..594fbdcf7cb3b3 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -419,7 +419,8 @@ private ActionResult execute( outErr, exitCode, startTime, - (int) wallTime.toMillis()); + (int) wallTime.toMillis(), + /* preserveExecutableBit= */ false); result = manifest.upload(context, cache, NullEventHandler.INSTANCE); } catch (ExecException e) { if (errStatus == null) { From d48e77c97ad1ea236169276c83c5b5e32dbcff9d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 7 Jun 2026 18:13:36 +0200 Subject: [PATCH 3/5] Comment --- .../build/lib/remote/AbstractActionInputPrefetcher.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index ae348d0b3510f9..141d198c23d95e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -83,10 +83,8 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet private final TempPathGenerator tempPathGenerator; private final OutputPermissions outputPermissions; - // The exec root may only be resolved once the workspace name is known, which does not happen - // until the loading phase. Since the remote repo contents cache materializes external repos - // before then (and only ever resolves paths under the external directory, which lives directly - // under the output base), exec root resolution is deferred via this supplier. + // The exec root isn't known when the prefetcher is created as its path depends on the name set in + // WORKSPACE. private final Supplier execRootSupplier; protected final Path outputBase; protected final RemoteOutputChecker remoteOutputChecker; From 9aad6ba7d1c91bc8227a42e5a682ad46cf0ffa11 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 7 Jun 2026 18:15:35 +0200 Subject: [PATCH 4/5] Simplify --- .../remote/AbstractActionInputPrefetcher.java | 23 ------------------- .../RemoteExternalOverlayFileSystem.java | 4 +--- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 141d198c23d95e..690d430e275f4c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -337,29 +337,6 @@ public ListenableFuture prefetchFiles( MetadataSupplier metadataSupplier, Priority priority, Reason reason) { - return prefetchFilesInterruptibly(action, inputs, metadataSupplier, priority, reason); - } - - /** - * Fetches remotely stored action outputs and stores them under their path in the output base. - * - *

The {@code inputs} may not contain any unexpanded directories. - * - *

This method is safe to be called concurrently from spawn runners before running any local - * spawn. - * - *

This method is similar to #prefetchFiles() above, but note that {@code metadataSupplier} may - * throw {@link InterruptedException}. If it does, this method will propagate this exception in - * the returned future. - * - * @return a future that is completed once all downloads have finished. - */ - public ListenableFuture prefetchFilesInterruptibly( - @Nullable ActionExecutionMetadata action, - Iterable inputs, - MetadataSupplier metadataSupplier, - Priority priority, - Reason reason) { List files = new ArrayList<>(); for (ActionInput input : inputs) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java index fac4b90300b74e..89c7b61f75b256 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java @@ -54,7 +54,6 @@ import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.SkyFunctionException; import java.io.ByteArrayInputStream; -import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.InterruptedIOException; @@ -273,7 +272,7 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) var walkResult = walk(remoteRepo); var unused = getFromFuture( - inputPrefetcher.prefetchFilesInterruptibly( + inputPrefetcher.prefetchFiles( /* action= */ null, Iterables.transform( walkResult.files(), path -> ActionInputHelper.fromPath(path.asFragment())), @@ -566,7 +565,6 @@ public void chmod(PathFragment path, int mode) throws IOException { fsForPath(path).chmod(path, mode); } - private final class RemoteExternalFileSystem extends RemoteActionFileSystem.RemoteInMemoryFileSystem { From cfa0f7d24b769023ddd6aadb04893a8653ab805c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 17 Jun 2026 10:52:13 +0200 Subject: [PATCH 5/5] Fix comments --- .../remote/AbstractActionInputPrefetcher.java | 24 ++++++++----------- .../skyframe/ActionOutputMetadataStore.java | 3 +-- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 690d430e275f4c..10813dce52bbc3 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -47,12 +47,12 @@ import com.google.devtools.build.lib.actions.FileContentsProxy; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; -import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.FileSymlinkLoopException; @@ -248,12 +248,9 @@ protected Path execRoot() { } /** - * Resolves an exec path to an absolute path. On 8.7.0, external repos are at - * output_base/external/ (sibling of execroot/), so exec paths starting with "external/" must be - * resolved relative to the output base rather than the exec root. - * - *

Absolute paths (e.g., from RemoteExternalOverlayFileSystem.prefetch) are returned as-is on - * the exec root's file system. + * Resolves an exec path to an absolute path, avoiding evaluation of execRoot() if possible as it + * isn't available during server startup. This logic is unique to Bazel 8.x as it still names the + * exec root based on the name set in WORKSPACE, which is gone from HEAD and Bazel 9.x. */ private Path resolveExecPath(PathFragment execPath) { if (execPath.isAbsolute()) { @@ -265,8 +262,7 @@ private Path resolveExecPath(PathFragment execPath) { return execRoot().getRelative(execPath); } - private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) - throws IOException { + private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) throws IOException { var stat = path.statIfFound(); if (stat == null) { return true; @@ -601,14 +597,13 @@ private Completable downloadFileNoCheckRx( } // Downloads should always be written to the "actual" host file system, not any overlays. + // See the comment on resolveExecPath for the rationale behind the branching below. Path finalPath = path.forHostFileSystem(); PathFragment execPath; if (finalPath .asFragment() - .startsWith(outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { - // On 8.7.0, external repos are at output_base/external/ which is not under execRoot - // (output_base/execroot/_main/). Use the path relative to the output base instead. This also - // avoids resolving the exec root during external repo materialization, before it is known. + .startsWith( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { execPath = finalPath.asFragment().relativeTo(outputBase.asFragment()); } else { execPath = finalPath.asFragment().relativeTo(execRoot().asFragment()); @@ -673,7 +668,8 @@ private void finalizeDownload( // overlaying the host file system where the download is written to. if (!finalPath .asFragment() - .startsWith(outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { + .startsWith( + outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION).asFragment())) { // Ensure the parent directory exists and is writable. We cannot rely on this precondition to // have been established by the execution of the owning action in a previous invocation, since // the output tree may have been externally modified in between invocations. 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 c1fcd687ca2ca3..3f718a431d19fd 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 @@ -134,13 +134,12 @@ private ActionOutputMetadataStore( /** * Relativizes the given path against the exec root, falling back to the output base for paths - * not under the exec root (e.g., external repos on 8.7.0 which are at output_base/external/). + * not under the exec root. */ private PathFragment relativizeToExecRootOrOutputBase(PathFragment path) { if (path.startsWith(execRoot)) { return path.relativeTo(execRoot); } - // External repos on 8.7.0 are at output_base/external/, a sibling of execroot/. return path.relativeTo(execRoot.getParentDirectory().getParentDirectory()); }