From 40024d57e503e683c3dc1eb38970e2b944232319 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 7 Jan 2026 02:04:51 -0800 Subject: [PATCH 1/3] [8.8.0] Prefetch `.bzl` files in the remote repo contents cache `.bzl` files are typically small, but can form deep DAGs that require a large number of sequential cache requests to fetch lazily. By prefetching them (as well as `REPO.bazel` files) eagerly, the wall time of one particular fully cached cold `--nobuild` build of Bazel itself decreased by a factor of 5. Along the way, make remote repo contents cache failures non-fatal, matching the behavior of the remote cache. Closes #27910. PiperOrigin-RevId: 853153815 Change-Id: I368a14a845a8d9fb543f473d8c0c2178a4590c78 (cherry picked from commit 361c4204682393b7bf8ac3b6f67c26e9cc746dcf) --- .../remote/AbstractActionInputPrefetcher.java | 5 +- .../RemoteExternalOverlayFileSystem.java | 106 ++++++++++++------ .../remote/RemoteRepoContentsCacheImpl.java | 3 +- .../RepositoryDelegatorFunction.java | 11 +- .../bzlmod/remote_repo_contents_cache_test.py | 75 +++++++++++++ 5 files changed, 158 insertions(+), 42 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 10813dce52bbc3..45c114ea2cdadc 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 @@ -269,8 +269,9 @@ private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) throws } // If an action output is stale, Skyframe will delete it prior to action execution. However, - // this doesn't apply to spawn outputs that aren't action outputs. To avoid incorrectly reusing - // one such stale output, check for its up-to-dateness here. + // this doesn't apply to spawn outputs that aren't action outputs, or to files in external repos + // that are remote repo contents cache hits. To avoid incorrectly reusing one such stale file, + // check for its up-to-dateness here. if (stat.getSize() != metadata.getSize()) { return true; } 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 39fdaf35d58744..d89181fa2c999e 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 @@ -26,7 +26,7 @@ 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.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -68,6 +68,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.function.Consumer; import javax.annotation.Nullable; /** @@ -79,16 +80,6 @@ */ 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; @@ -189,31 +180,54 @@ public void afterCommand() { this.evaluator = null; } - public void injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile) - throws IOException { + /** + * Injects the given remote contents, possibly prefetching some files, and returns true on + * success. + */ + public boolean injectRemoteRepo(RepositoryName repo, Tree remoteContents, String markerFile) + throws IOException, InterruptedException { var childMap = remoteContents.getChildrenList().stream() .collect( toImmutableMap(cache.digestUtil::compute, directory -> directory, (a, b) -> a)); + var repoDir = externalDirectory.getChild(repo.getName()); + var filesToPrefetch = new ArrayList(); injectRecursively( - externalFs, externalDirectory.getChild(repo.getName()), remoteContents.getRoot(), childMap); + externalFs, repoDir, remoteContents.getRoot(), childMap, filesToPrefetch::add); + try { + // TODO: This prefetches a large number of small files. Investigate whether BatchReadBlobs + // would be more efficient. + prefetch(filesToPrefetch); + } catch (BulkTransferException e) { + if (e.allCausedByCacheNotFoundException()) { + // The cache has lost the .bzl files, which should be treated just like a cache miss. + externalFs.deleteTree(repoDir); + return false; + } + throw e; + } // 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); + return true; } private static void injectRecursively( RemoteExternalFileSystem fs, PathFragment path, Directory dir, - ImmutableMap childMap) + ImmutableMap childMap, + Consumer filesToPrefetch) throws IOException { fs.createDirectoryAndParents(path); for (var file : dir.getFilesList()) { var filePath = path.getRelative(unicodeToInternal(file.getName())); + if (shouldPrefetch(filePath)) { + filesToPrefetch.accept(filePath); + } fs.injectFile( filePath, FileArtifactValue.RemoteFileArtifactValue.create( @@ -238,7 +252,7 @@ private static void injectRecursively( "Directory %s with digest %s not found in tree" .formatted(subdirPath, subdirNode.getDigest().getHash())); } - injectRecursively(fs, subdirPath, subdir, childMap); + injectRecursively(fs, subdirPath, subdir, childMap, filesToPrefetch); } } @@ -276,22 +290,15 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) var remoteRepo = externalFs.getPath(repoPath); var walkResult = walk(remoteRepo); for (var directory : walkResult.directories()) { - nativeFs.getPath(directory.asFragment()).createDirectory(); + nativeFs.getPath(directory).createDirectory(); } - var unused = - getFromFuture( - inputPrefetcher.prefetchFiles( - /* action= */ null, - Iterables.transform( - walkResult.files(), path -> ActionInputHelper.fromPath(path.asFragment())), - actionInput -> externalFs.getMetadata(actionInput.getExecPath()), - ActionInputPrefetcher.Priority.CRITICAL, - ActionInputPrefetcher.Reason.INPUTS)); + prefetch(walkResult.files()); // Create symlinks last as some platforms don't allow creating a symlink to a non-existent // target. A symlink may have already been created as an input to an action. for (var remoteSymlink : walkResult.symlinks()) { - var nativeSymlink = nativeFs.getPath(remoteSymlink.asFragment()); - FileSystemUtils.ensureSymbolicLink(nativeSymlink, remoteSymlink.readSymbolicLink()); + var nativeSymlink = nativeFs.getPath(remoteSymlink); + FileSystemUtils.ensureSymbolicLink( + nativeSymlink, externalFs.getPath(remoteSymlink).readSymbolicLink()); } // After the repo has been copied, atomically materialize the marker file. This ensures that the @@ -304,7 +311,19 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) markerFileSibling.renameTo(markerFile); } - private record WalkResult(List files, List symlinks, List directories) {} + private void prefetch(List paths) throws IOException, InterruptedException { + var unused = + getFromFuture( + inputPrefetcher.prefetchFiles( + /* action= */ null, + Lists.transform(paths, ActionInputHelper::fromPath), + actionInput -> externalFs.getMetadata(actionInput.getExecPath()), + ActionInputPrefetcher.Priority.CRITICAL, + ActionInputPrefetcher.Reason.INPUTS)); + } + + private record WalkResult( + List files, List symlinks, List directories) {} private static WalkResult walk(Path root) throws IOException { var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); @@ -316,10 +335,10 @@ 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 FILE -> result.files.add(fromChild.asFragment()); + case SYMLINK -> result.symlinks.add(fromChild.asFragment()); case DIRECTORY -> { - result.directories.add(fromChild); + result.directories.add(fromChild.asFragment()); walk(fromChild, result); } default -> throw new IOException("Unsupported file type: " + dirent); @@ -327,6 +346,26 @@ private static void walk(Path root, WalkResult result) throws IOException { } } + /** Whether the file with the given path should be materialized eagerly when injecting a repo. */ + private static boolean shouldPrefetch(PathFragment path) { + // .bzl files are typically small and the loads between them can form complex DAGs that can only + // be discovered layer by layer, so prefetching is worthwhile to reduce the number of sequential + // cache requests. + // The REPO.bazel file, if present, is a dependency of any package and will thus have to be + // fetched anyway. + return path.getFileExtension().equals("bzl") || path.getBaseName().equals("REPO.bazel"); + } + + @Override + public FileSystem getHostFileSystem() { + return nativeFs.getHostFileSystem(); + } + + @Override + public boolean isFilePathCaseSensitive() { + return nativeFs.isFilePathCaseSensitive(); + } + // Always mirror tree deletions to the underlying native file system to support bazel clean and // repository refetching. @@ -603,6 +642,9 @@ private FileArtifactValue getMetadata(PathFragment path) throws IOException { @Override public synchronized InputStream getInputStream(PathFragment path) throws IOException { + if (shouldPrefetch(path)) { + return nativeFs.getInputStream(path); + } var relativePath = path.relativeTo(externalDirectory); var info = (RemoteActionFileSystem.RemoteInMemoryFileInfo) stat(path, /* followSymlinks= */ true); 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 index 2066cb9bb77909..ee89cc45062863 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java @@ -270,8 +270,7 @@ public boolean lookupCache( return false; } - remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent); - return true; + return remoteFs.injectRemoteRepo(repoName, repoDirectoryContent, markerFileContent); } private RemoteActionExecutionContext buildContext(RepositoryName repoName) { 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 958be029590d9a..12ec94bc977bf0 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 @@ -272,12 +272,11 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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); + env.getListener() + .handle( + Event.warn( + "Remote repo contents cache lookup failed for %s: %s" + .formatted(repositoryName, e.getMessage()))); } } } 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 index c901e9b216c900..7ebce79de93ee8 100644 --- a/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py @@ -641,6 +641,81 @@ def testLostRemoteFile_build(self): 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'))) + def testBzlFilePrefetching(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", """', + 'load(":nested.bzl", "nested_fg")', + 'nested_fg(name = "haha")', + '""")', + ' rctx.file("nested.bzl", """', + 'load("//subdir:more_nested.bzl", "more_nested_fg")', + 'def nested_fg(name):', + ' more_nested_fg(name = name)', + '""")', + ' rctx.file("subdir/BUILD")', + ' rctx.file("subdir/more_nested.bzl", """', + 'def more_nested_fg(name):', + ' native.filegroup(name = name)', + '""")', + ' rctx.file("file.txt", "hello")', + ' 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, 'file.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + + # After expunging: cached, .bzl files 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, 'file.txt'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + + # 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, 'file.txt'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/BUILD'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'nested.bzl'))) + self.assertTrue( + os.path.exists(os.path.join(repo_dir, 'subdir/more_nested.bzl')) + ) + if __name__ == '__main__': absltest.main() From 719c4abdcd071db1c59442e5d56eaaa061ebee14 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 7 Jan 2026 06:44:31 -0800 Subject: [PATCH 2/3] Show stack traces in the remote repo contents cache with `--verbose_failures` Makes it easier to debug issues with this experimental feature and also matches the behavior of remote execution/caching. Work towards #27965 Closes #27970. PiperOrigin-RevId: 853238791 Change-Id: Id46ccbb105d93fd17114fab13b086d0b46139fb4 (cherry picked from commit fc5f160593b5039f3cd10ae844e4eb7f5dce03b8) --- .../build/lib/remote/RemoteModule.java | 3 +- .../remote/RemoteRepoContentsCacheImpl.java | 33 ++++++++++++++++--- .../RepositoryRemoteHelpersFactoryImpl.java | 7 ++-- 3 files changed, 36 insertions(+), 7 deletions(-) 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 6fb1dd302d1926..7d4dc72c888cf1 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 @@ -768,7 +768,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { invocationId, remoteOptions.remoteInstanceName, remoteOptions.remoteAcceptCached, - remoteOptions.remoteUploadLocalResults)); + remoteOptions.remoteUploadLocalResults, + verboseFailures)); if (env.getDirectories().getOutputBase().getFileSystem() instanceof RemoteExternalOverlayFileSystem remoteFs) { remoteFs.beforeCommand( 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 index ee89cc45062863..c8a2c20a5f9fc5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java @@ -24,6 +24,7 @@ import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.Tree; import com.google.common.base.Splitter; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.Futures; @@ -95,6 +96,7 @@ public final class RemoteRepoContentsCacheImpl implements RemoteRepoContentsCach private final String commandId; private final boolean acceptCached; private final boolean uploadLocalResults; + private final boolean verboseFailures; private final DigestUtil digestUtil; private final Action baseAction; @@ -103,12 +105,14 @@ public RemoteRepoContentsCacheImpl( String buildRequestId, String commandId, boolean acceptCached, - boolean uploadLocalResults) { + boolean uploadLocalResults, + boolean verboseFailures) { this.buildRequestId = buildRequestId; this.commandId = commandId; this.cache = cache; this.acceptCached = acceptCached; this.uploadLocalResults = uploadLocalResults; + this.verboseFailures = verboseFailures; this.digestUtil = cache.digestUtil; this.baseAction = Action.newBuilder() @@ -151,7 +155,7 @@ public void addToCache( reporter.handle( Event.warn( "Failed to read marker file repo %s, skipping: %s" - .formatted(repoName, e.getMessage()))); + .formatted(repoName, maybeGetStackTrace(e)))); } var action = buildAction(predeclaredInputHash); var actionKey = new ActionKey(digestUtil.compute(action)); @@ -178,7 +182,7 @@ public void addToCache( reporter.handle( Event.warn( "Failed to upload repo contents to remote cache for repo %s: %s" - .formatted(repoName, e.getMessage()))); + .formatted(repoName, maybeGetStackTrace(e)))); } } @@ -189,6 +193,22 @@ public boolean lookupCache( String predeclaredInputHash, ExtendedEventHandler reporter) throws IOException, InterruptedException { + try { + return doLookupCache(repoName, repoDir, predeclaredInputHash, reporter); + } catch (IOException e) { + throw new IOException( + "Failed to look up repo %s in the remote repo contents cache: %s" + .formatted(repoName, maybeGetStackTrace(e)), + e); + } + } + + private boolean doLookupCache( + RepositoryName repoName, + Path repoDir, + String predeclaredInputHash, + ExtendedEventHandler reporter) + throws IOException, InterruptedException { if (!(repoDir.getFileSystem() instanceof RemoteExternalOverlayFileSystem remoteFs)) { return false; } @@ -246,7 +266,8 @@ public boolean lookupCache( markerFileContent = new String(markerFileContentFuture.get(), StandardCharsets.ISO_8859_1); repoDirectoryContent = repoDirectoryContentFuture.get(); } catch (ExecutionException e) { - throw new IllegalStateException("waitForBulkTransfer should have thrown", e); + throw new IllegalStateException( + "waitForBulkTransfer should have thrown: " + maybeGetStackTrace(e)); } var markerFileLines = Splitter.on('\n') @@ -294,6 +315,10 @@ private Action buildAction(String predeclaredInputHash) { .build(); } + private String maybeGetStackTrace(Exception e) { + return verboseFailures ? Throwables.getStackTraceAsString(e) : e.getMessage(); + } + private record RepoRemotePathResolver(Path fetchedRepoMarkerFile, Path fetchedRepoDir) implements RemotePathResolver { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java index 5d3d95a022534f..2b4035128f7b98 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RepositoryRemoteHelpersFactoryImpl.java @@ -30,6 +30,7 @@ class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFacto private final String remoteInstanceName; private final boolean acceptCached; private final boolean uploadLocalResults; + private final boolean verboseFailures; RepositoryRemoteHelpersFactoryImpl( CombinedCache cache, @@ -38,7 +39,8 @@ class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFacto String commandId, String remoteInstanceName, boolean acceptCached, - boolean uploadLocalResults) { + boolean uploadLocalResults, + boolean verboseFailures) { this.cache = cache; this.remoteExecutor = remoteExecutor; this.buildRequestId = buildRequestId; @@ -46,6 +48,7 @@ class RepositoryRemoteHelpersFactoryImpl implements RepositoryRemoteHelpersFacto this.remoteInstanceName = remoteInstanceName; this.acceptCached = acceptCached; this.uploadLocalResults = uploadLocalResults; + this.verboseFailures = verboseFailures; } @Nullable @@ -68,6 +71,6 @@ public RepositoryRemoteExecutor createExecutor() { @Override public RemoteRepoContentsCache createRepoContentsCache() { return new RemoteRepoContentsCacheImpl( - cache, buildRequestId, commandId, acceptCached, uploadLocalResults); + cache, buildRequestId, commandId, acceptCached, uploadLocalResults, verboseFailures); } } From 8e5a8aac275b61c6c697ff5f7081298ec6858356 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 8 Jan 2026 01:40:14 -0800 Subject: [PATCH 3/3] Fix repo contents cache `FileValue` staleness Ensures that files under repo contents cache entries are not reported as missing after the cache has been deleted while the Bazel server is running. See the long comment in `RepositoryFetchFunction` for why this happens and how it is fixed. Fixes #26450 Closes #28147. PiperOrigin-RevId: 853622194 Change-Id: Ifba953b72258030e0a640ac49947ac5c5fc7620a (cherry picked from commit 7019132cadb4e2e5d84fa4e1cf235e9123264281) --- .../lib/bazel/BazelRepositoryModule.java | 1 + .../RepositoryDelegatorFunction.java | 33 +++- .../build/lib/runtime/WorkspaceBuilder.java | 10 ++ .../lib/skyframe/DirtinessCheckerUtils.java | 1 + .../lib/skyframe/ExternalFilesHelper.java | 56 ++++++- .../skyframe/SequencedSkyframeExecutor.java | 12 ++ .../SequencedSkyframeExecutorFactory.java | 4 + .../build/lib/skyframe/SkyframeExecutor.java | 4 +- .../lib/skyframe/SkyframeExecutorFactory.java | 3 + .../packages/AbstractPackageLoader.java | 6 +- ...ractCollectPackagesUnderDirectoryTest.java | 1 + src/test/py/bazel/BUILD | 1 + .../bazel/bzlmod/repo_contents_cache_test.py | 148 +++++++++++++++++- 13 files changed, 266 insertions(+), 14 deletions(-) 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 77af5a4afb76eb..98c8ac2f3bc402 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 @@ -249,6 +249,7 @@ public void workspaceInit( SkyframeExecutorRepositoryHelpersHolder.create( new RepositoryDirectoryDirtinessChecker())); } + builder.setRepoContentsCachePathSupplier(repositoryCache.getRepoContentsCache()::getPath); // Create the repository function everything flows through. repositoryDelegatorFunction = 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 12ec94bc977bf0..bd8a1003028172 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 @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Table; +import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; @@ -312,16 +313,40 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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. + // Don't forget to register a FileStateValue on the cache repo dir, so that we know to + // refetch if the cache entry gets GC'd from under us or the entire cache is deleted. + // + // Note that registering a FileValue dependency instead would lead to subtly incorrect + // behavior when the repo contents cache directory is deleted between builds: + // 1. We register a FileValue dependency on the cache entry. + // 2. Before the next build, the repo contents cache directory is deleted. + // 3. On the next build, FileSystemValueChecker invalidates the underlying + // FileStateValue, which in turn results in the FileValue and the current + // RepositoryDirectoryValue being marked as dirty. + // 4. Skyframe visits the dirty nodes bottom up to check for actual changes. In + // particular, it reevaluates FileFunction before RepositoryFetchFunction and thus + // the FileValue of the repo contents cache directory is locked in as non-existent + // before RepositoryFetchFunction can recreate it. + // 5. Any other SkyFunction that depends on the FileValue of a file in the repo (e.g. + // PackageFunction) will report that file as missing since the resolved path has a + // parent that is non-existent. + // By using FileStateValue directly, which benefits from special logic built into + // DirtinessCheckerUtils that recognizes the repo contents cache directories with + // non-UUID names and prevents locking in their value during dirtiness checking, we + // avoid 4. and thus the incorrect missing file errors in 5. if (env.getValue( - FileValue.key( + FileStateValue.key( RootedPath.toRootedPath( Root.absoluteRoot(cachedRepoDir.getFileSystem()), cachedRepoDir))) == null) { return null; } + // This is never reached: the repo dir in the repo contents cache is created under a new + // UUID-named directory and thus the FileStateValue above will always be missing from + // Skyframe. After the restart, the repo will either encounter the just created cache + // entry as a candidate or will create a new one if it got GC'd in the meantime. + throw new IllegalStateException( + "FileStateValue unexpectedly present for " + cachedRepoDir); } if (remoteRepoContentsCache != null) { remoteRepoContentsCache.addToCache( diff --git a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java index bc6b10eb21dad3..9c000a1e957f09 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueService; import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecRegistry; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SingleFileSystemSyscallCache; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; @@ -66,6 +67,7 @@ public final class WorkspaceBuilder { private SyscallCache syscallCache; private boolean allowExternalRepositories = true; + private Supplier repoContentsCachePathSupplier = () -> null; @Nullable private Supplier analysisCodecRegistrySupplier = null; @Nullable private FingerprintValueService.Factory fingerprintValueServiceFactory = null; @@ -125,6 +127,7 @@ BlazeWorkspace build( skyFunctions.buildOrThrow(), singleFsSyscallCache, skyframeExecutorRepositoryHelpersHolder, + repoContentsCachePathSupplier, skyKeyStateReceiver == null ? SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE : skyKeyStateReceiver, @@ -228,6 +231,13 @@ public WorkspaceBuilder setAllowExternalRepositories(boolean allowExternalReposi return this; } + @CanIgnoreReturnValue + public WorkspaceBuilder setRepoContentsCachePathSupplier( + Supplier repoContentsCachePathSupplier) { + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; + return this; + } + @CanIgnoreReturnValue public WorkspaceBuilder setSkyKeyStateReceiver( SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java index f321be30b265ed..fd68caaf19fb8b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java @@ -165,6 +165,7 @@ private static boolean isCacheableType(FileType fileType) { return true; case EXTERNAL_REPO: case OUTPUT: + case REPO_CONTENTS_CACHE_DIRS: return false; } throw new AssertionError("Unknown type " + fileType); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index ca8293f9c6d736..e3d59041344bda 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java @@ -33,6 +33,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; /** Common utilities for dealing with paths outside the package roots. */ public class ExternalFilesHelper { @@ -42,6 +43,7 @@ public class ExternalFilesHelper { private final ExternalFileAction externalFileAction; private final BlazeDirectories directories; private final int maxNumExternalFilesToLog; + private final Supplier repoContentsCachePathSupplier; private final AtomicInteger numExternalFilesLogged = new AtomicInteger(0); private static final int MAX_EXTERNAL_FILES_TO_TRACK = 2500; @@ -58,31 +60,52 @@ private ExternalFilesHelper( AtomicReference pkgLocator, ExternalFileAction externalFileAction, BlazeDirectories directories, + Supplier repoContentsCachePathSupplier, int maxNumExternalFilesToLog) { this.pkgLocator = pkgLocator; this.externalFileAction = externalFileAction; this.directories = directories; + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; this.maxNumExternalFilesToLog = maxNumExternalFilesToLog; } public static ExternalFilesHelper create( AtomicReference pkgLocator, ExternalFileAction externalFileAction, - BlazeDirectories directories) { + BlazeDirectories directories, + Supplier repoContentsCachePathSupplier) { return TestType.isInTest() - ? createForTesting(pkgLocator, externalFileAction, directories) + ? createForTesting( + pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier) : new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, /*maxNumExternalFilesToLog=*/ 100); + pkgLocator, + externalFileAction, + directories, + repoContentsCachePathSupplier, + /* maxNumExternalFilesToLog= */ 100); } public static ExternalFilesHelper createForTesting( AtomicReference pkgLocator, ExternalFileAction externalFileAction, BlazeDirectories directories) { + return createForTesting( + pkgLocator, + externalFileAction, + directories, + /* repoContentsCachePathSupplier= */ () -> null); + } + + public static ExternalFilesHelper createForTesting( + AtomicReference pkgLocator, + ExternalFileAction externalFileAction, + BlazeDirectories directories, + Supplier repoContentsCachePathSupplier) { return new ExternalFilesHelper( pkgLocator, externalFileAction, directories, + repoContentsCachePathSupplier, // These log lines are mostly spam during unit and integration tests. /*maxNumExternalFilesToLog=*/ 0); } @@ -153,6 +176,20 @@ public enum FileType { * RepositoryDirectoryValue is computed. */ EXTERNAL_REPO, + + /** + * The directory containing the repo contents cache entries as well as direct children + * corresponding to individual predeclared input hashes. These directories are created by Bazel + * but may be deleted when users delete the entire repo contents cache. + * + *

These files' handling differs from EXTERNAL_REPO as they are never modified after they are + * created and don't live under the external directory, as well as from EXTERNAL as they + * can be recreated by Bazel after diff detection. + * + *

The contents of these directories are considered EXTERNAL as they carry UUID names + * and are thus never reused. + */ + REPO_CONTENTS_CACHE_DIRS, } /** @@ -199,7 +236,11 @@ void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) { ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() { return new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, maxNumExternalFilesToLog); + pkgLocator, + externalFileAction, + directories, + repoContentsCachePathSupplier, + maxNumExternalFilesToLog); } public FileType getAndNoteFileType(RootedPath rootedPath) { @@ -236,6 +277,12 @@ private FileType detectFileType(RootedPath rootedPath) { if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) { return FileType.INTERNAL; } + var repoContentsCachePath = repoContentsCachePathSupplier.get(); + if (repoContentsCachePath != null + && rootedPath.asPath().startsWith(repoContentsCachePath) + && !rootedPath.asPath().relativeTo(repoContentsCachePath).isMultiSegment()) { + return FileType.REPO_CONTENTS_CACHE_DIRS; + } // The outputBase may be null if we're not actually running a build. Path outputBase = packageLocator.getOutputBase(); if (outputBase == null) { @@ -268,6 +315,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment switch (fileType) { case BUNDLED: case INTERNAL: + case REPO_CONTENTS_CACHE_DIRS: break; case EXTERNAL: if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 43820a3c6fc0a5..973f6313718ddb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -79,6 +79,7 @@ import com.google.devtools.build.lib.vfs.FileStateKey; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.DelegatingGraphInconsistencyReceiver; import com.google.devtools.build.skyframe.EmittedEventState; @@ -113,6 +114,7 @@ import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; +import java.util.function.Supplier; import javax.annotation.Nullable; /** @@ -164,6 +166,7 @@ protected SequencedSkyframeExecutor( ImmutableList buildFilesByPriority, ExternalPackageHelper externalPackageHelper, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile, boolean shouldUseRepoDotBazel, SkyKeyStateReceiver skyKeyStateReceiver, @@ -194,6 +197,7 @@ protected SequencedSkyframeExecutor( workspaceInfoFromDiffReceiver, new SequencedRecordingDifferencer(), repositoryHelpersHolder, + repoContentsCachePathSupplier, globUnderSingleDep); } @@ -799,6 +803,7 @@ public static final class Builder { private WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver = (ignored1, ignored2) -> {}; @Nullable private SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder = null; + private Supplier repoContentsCachePathSupplier = () -> null; private Consumer skyframeExecutorConsumerOnInit = skyframeExecutor -> {}; private SkyFunction ignoredPackagePrefixesFunction; private BugReporter bugReporter = BugReporter.defaultInstance(); @@ -837,6 +842,7 @@ public SequencedSkyframeExecutor build() { buildFilesByPriority, externalPackageHelper, repositoryHelpersHolder, + repoContentsCachePathSupplier, actionOnIOExceptionReadingBuildFile, shouldUseRepoDotBazel, skyKeyStateReceiver, @@ -916,6 +922,12 @@ public Builder setRepositoryHelpersHolder( return this; } + @CanIgnoreReturnValue + public Builder setRepoContentsCachePathSupplier(Supplier repoContentsCachePathSupplier) { + this.repoContentsCachePathSupplier = repoContentsCachePathSupplier; + return this; + } + @CanIgnoreReturnValue public Builder setCrossRepositoryLabelViolationStrategy( CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java index 65fda322698437..ac929ffcee429e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java @@ -20,9 +20,11 @@ import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; +import java.util.function.Supplier; import javax.annotation.Nullable; /** A factory of SkyframeExecutors that returns SequencedSkyframeExecutor. */ @@ -39,6 +41,7 @@ public SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver, BugReporter bugReporter) { return BazelSkyframeExecutorConstants.newBazelSkyframeExecutorBuilder() @@ -51,6 +54,7 @@ public SkyframeExecutor create( .setExtraSkyFunctions(extraSkyFunctions) .setSyscallCache(syscallCache) .setRepositoryHelpersHolder(repositoryHelpersHolder) + .setRepoContentsCachePathSupplier(repoContentsCachePathSupplier) .setSkyKeyStateReceiver(skyKeyStateReceiver) .setBugReporter(bugReporter) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index f57e51c8af78b0..0eb6797f0b275a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -566,6 +566,7 @@ protected SkyframeExecutor( @Nullable WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver, @Nullable RecordingDifferencer recordingDiffer, @Nullable SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, boolean globUnderSingleDep) { // Strictly speaking, these arguments are not required for initialization, but all current // callsites have them at hand, so we might as well set them during construction. @@ -609,7 +610,8 @@ protected SkyframeExecutor( this.skyframeBuildView = new SkyframeBuildView(artifactFactory, this, ruleClassProvider, actionKeyContext); this.externalFilesHelper = - ExternalFilesHelper.create(pkgLocator, externalFileAction, directories); + ExternalFilesHelper.create( + pkgLocator, externalFileAction, directories, repoContentsCachePathSupplier); this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy; this.buildFilesByPriority = buildFilesByPriority; this.externalPackageHelper = externalPackageHelper; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java index ce922b4676b56f..aee3b3fa16ad9c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java @@ -21,9 +21,11 @@ import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; +import java.util.function.Supplier; /** * A factory that creates instances of SkyframeExecutor. @@ -52,6 +54,7 @@ SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, SkyframeExecutorRepositoryHelpersHolder repositoryHelpersHolder, + Supplier repoContentsCachePathSupplier, SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver, BugReporter bugReporter) throws AbruptExitException; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 383eaabb377801..c3f4a3d80a179e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -283,7 +283,11 @@ protected void validate() { public final PackageLoader build() { validate(); externalFilesHelper = - ExternalFilesHelper.create(pkgLocatorRef, externalFileAction, directories); + ExternalFilesHelper.create( + pkgLocatorRef, + externalFileAction, + directories, + /* repoContentsCachePathSupplier= */ () -> null); return buildImpl(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java index e570d188df408f..8e40ed66dd66b8 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java @@ -313,6 +313,7 @@ private void initEvaluator() getExtraSkyFunctions(), SyscallCache.NO_CACHE, /* repositoryHelpersHolder= */ null, + /* repoContentsCachePathSupplier= */ () -> null, SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE, BugReporter.defaultInstance()); skyframeExecutor.injectExtraPrecomputedValues( diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD index 60991b26682fa1..97290e73a9e179 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -415,6 +415,7 @@ py_test( name = "repo_contents_cache_test", size = "large", srcs = ["bzlmod/repo_contents_cache_test.py"], + shard_count = 4, tags = ["requires-network"], deps = [ ":bzlmod_test_utils", diff --git a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py index f76fb6654d3162..961323f952594a 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -14,7 +14,9 @@ # limitations under the License. # pylint: disable=g-long-ternary +import json import os +import pathlib import tempfile import time @@ -54,6 +56,18 @@ def sleepUntilCacheEmpty(self): time.sleep(0.5) self.fail('repo contents cache still not empty after 5 seconds') + 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', @@ -76,6 +90,21 @@ def testCachedAfterCleanExpunge(self): # First fetch: not cached _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) self.assertIn('JUST FETCHED', '\n'.join(stderr)) + # Verify that the repo directory under the output base is a symlink or + # junction into the repo contents cache. + repo_dir = self.repoDir('my_repo') + self.assertTrue(os.path.islink(repo_dir) or os.path.isjunction(repo_dir)) + target_path = os.readlink(repo_dir) + real_target_path = os.path.realpath(target_path) + real_repo_contents_cache = os.path.realpath(self.repo_contents_cache) + for parent in pathlib.Path(real_target_path).parents: + if parent.samefile(real_repo_contents_cache): + break + else: + self.fail( + 'repo target dir %s is not in the repo contents cache %s' + % (real_target_path, real_repo_contents_cache) + ) # After expunging: cached self.RunBazel(['clean', '--expunge']) @@ -295,7 +324,9 @@ def testGc_singleServer_gcAfterCacheHit(self): # GC'd while server is alive: not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) def testGc_singleServer_gcAfterCacheMiss(self): self.ScratchFile( @@ -328,7 +359,9 @@ def testGc_singleServer_gcAfterCacheMiss(self): # GC'd while server is alive: not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha']) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) def testGc_multipleServers(self): module_bazel_lines = [ @@ -383,13 +416,17 @@ def testGc_multipleServers(self): ], cwd=dir_a, ) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) # GC'd while B's server is alive (after B's earlier cache hit): # not cached, but also no crash self.sleepUntilCacheEmpty() _, _, stderr = self.RunBazel(['build', '@my_repo//:haha'], cwd=dir_b) - self.assertIn('JUST FETCHED', '\n'.join(stderr)) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) def testReverseDependencyDirection(self): # Set up two repos that retain their predeclared input hashes across two @@ -450,6 +487,109 @@ def testReverseDependencyDirection(self): self.AssertNotExitCode(exit_code, 0, stderr, stdout) self.assertIn('possible dependency cycle detected', '\n'.join(stderr)) + def doTestRepoContentsCacheDeleted(self, check_external_repository_files): + repo_contents_cache = self.ScratchDir('repo_contents_cache') + workspace = self.ScratchDir('workspace') + extra_args = [ + '--experimental_check_external_repository_files=%s' + % str(check_external_repository_files).lower(), + '--repo_contents_cache=%s' % repo_contents_cache, + ] + + self.ScratchFile( + 'workspace/MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile('workspace/BUILD.bazel') + self.ScratchFile( + 'workspace/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)', + ], + ) + # First fetch: not cached + _, _, stderr = self.RunBazel( + [ + 'build', + '@my_repo//:haha', + ] + + extra_args, + cwd=workspace, + ) + self.assertIn('JUST FETCHED', '\n'.join(stderr)) + + # Second fetch: cached + _, _, stderr = self.RunBazel( + [ + 'build', + '@my_repo//:haha', + ] + + extra_args, + cwd=workspace, + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + + # Delete the entire repo contents cache and fetch again: not cached + # Avoid access denied on Windows due to files being read-only by moving to + # a different location instead. + os.rename(repo_contents_cache, repo_contents_cache + '_deleted') + _, _, stderr = self.RunBazel( + [ + 'build', + '@my_repo//:haha', + ] + + extra_args, + cwd=workspace, + ) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) + + # Second fetch after deletion: cached + _, _, stderr = self.RunBazel( + [ + 'build', + '@my_repo//:haha', + ] + + extra_args, + cwd=workspace, + ) + self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + self.assertNotIn('WARNING', '\n'.join(stderr)) + + # Delete the entire repo contents cache and fetch again with a different + # path: not cached + # Avoid access denied on Windows due to files being read-only by moving to + # a different location instead. + os.rename(repo_contents_cache, repo_contents_cache + '_deleted_again') + _, _, stderr = self.RunBazel( + [ + 'build', + '@my_repo//:haha', + ] + + extra_args + + [ + '--repo_contents_cache=%s' % repo_contents_cache + '2', + ], + cwd=workspace, + ) + stderr = '\n'.join(stderr) + self.assertIn('JUST FETCHED', stderr) + self.assertNotIn('WARNING', stderr) + + def testRepoContentsCacheDeleted_withCheckExternalRepositoryFiles(self): + self.doTestRepoContentsCacheDeleted(check_external_repository_files=True) + + def testRepoContentsCacheDeleted_withoutCheckExternalRepositoryFiles(self): + self.doTestRepoContentsCacheDeleted(check_external_repository_files=False) + if __name__ == '__main__': absltest.main()