From 52324fa454f1785e60f2957a28b8cd51079e7ec9 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 8 Jan 2026 01:40:14 -0800 Subject: [PATCH] [9.0.0] 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 --- .../lib/bazel/BazelRepositoryModule.java | 6 +- .../repository/RepositoryFetchFunction.java | 33 +++- .../build/lib/runtime/WorkspaceBuilder.java | 10 ++ .../lib/skyframe/DirtinessCheckerUtils.java | 2 +- .../lib/skyframe/ExternalFilesHelper.java | 71 +++++++-- .../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, 274 insertions(+), 27 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 5dbd6702309363..f60177c23913da 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 @@ -208,10 +208,8 @@ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builde @Override public void workspaceInit( BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { - // TODO(b/27143724): Remove this guard when Google-internal flavor no longer uses repositories. - if ("bazel".equals(runtime.getProductName())) { - builder.allowExternalRepositories(true); - } + builder.allowExternalRepositories(true); + builder.setRepoContentsCachePathSupplier(repositoryCache.getRepoContentsCache()::getPath); repositoryFetchFunction = new RepositoryFetchFunction( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java index 7e49e2b505cbe4..bfbab75b949330 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java @@ -21,6 +21,7 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +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.NonRegistryOverride; @@ -297,16 +298,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 297ba824b147d6..a5dab67d48dd03 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 @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecRegistry; import com.google.devtools.build.lib.skyframe.serialization.analysis.RemoteAnalysisCachingServicesSupplier; 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; @@ -62,6 +63,7 @@ public final class WorkspaceBuilder { private SyscallCache syscallCache; private boolean allowExternalRepositories = true; + private Supplier repoContentsCachePathSupplier = () -> null; @Nullable private Supplier analysisCodecRegistrySupplier = null; @Nullable @@ -123,6 +125,7 @@ BlazeWorkspace build( skyFunctions.buildOrThrow(), singleFsSyscallCache, allowExternalRepositories, + repoContentsCachePathSupplier, skyKeyStateReceiver == null ? SkyframeExecutor.SkyKeyStateReceiver.NULL_INSTANCE : skyKeyStateReceiver, @@ -223,6 +226,13 @@ public WorkspaceBuilder allowExternalRepositories(boolean allowExternalRepositor 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 129cdbe7550bb5..e4c2f995bed91e 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 @@ -167,7 +167,7 @@ Map getDirtyExternalRepos() { private static boolean isCacheableType(FileType fileType) { return switch (fileType) { case INTERNAL, EXTERNAL_OTHER, BUNDLED -> true; - case EXTERNAL_REPO, OUTPUT -> false; + case EXTERNAL_REPO, OUTPUT, REPO_CONTENTS_CACHE_DIRS -> false; }; } } 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 f238e1fb86b7b6..79e115b1a9267d 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 @@ -40,6 +40,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import javax.annotation.Nullable; /** Common utilities for dealing with paths outside the package roots. */ @@ -50,6 +51,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; @@ -66,31 +68,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); } @@ -155,11 +178,26 @@ public enum FileType { */ 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_OTHER as they + * can be recreated by Bazel after diff detection. + * + *

The contents of these directories are considered EXTERNAL_OTHER as they carry UUID names + * and are thus never reused. + */ + REPO_CONTENTS_CACHE_DIRS, + /** * None of the above. We encounter these paths when outputs, source files or external repos * symlink to files outside aforementioned Bazel-managed directories. For example, C compilation * by the host compiler may depend on /usr/bin/gcc. Bazel makes a best-effort attempt to detect - * changes in such files. + * changes in such files. This type also includes files in repo contents cache entries, which + * are created by Bazel but never modified after creation. */ EXTERNAL_OTHER, } @@ -207,7 +245,11 @@ void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) { ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() { return new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, maxNumExternalFilesToLog); + pkgLocator, + externalFileAction, + directories, + repoContentsCachePathSupplier, + maxNumExternalFilesToLog); } public FileType getAndNoteFileType(RootedPath rootedPath) { @@ -240,6 +282,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) { @@ -270,6 +318,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment switch (fileType) { case BUNDLED: case INTERNAL: + case REPO_CONTENTS_CACHE_DIRS: break; case EXTERNAL_OTHER: if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) { @@ -294,14 +343,12 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment /** * For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding repo so - * that if the repo definition changes, the File/DirectoryStateValue will be re-evaluated. + * that if the repo is refetched, the {File,DirectoryListing}StateValue's of files underneath will + * be re-evaluated. * - *

Note that: - We don't add a dependency on the parent directory at the package root boundary, - * so the only transitive dependencies from files inside the package roots to external files are - * through symlinks. So the upwards transitive closure of external files is small. - The only way - * other than external repositories for external source files to get into the skyframe graph in - * the first place is through symlinks outside the package roots, which we neither want to - * encourage nor optimize for since it is not common. So the set of external files is small. + *

Note that we don't add a dependency on the parent directory at the package root boundary, so + * the only transitive dependencies from files inside the package roots to external files are + * through symlinks. So the upwards transitive closure of external files is small. */ private void addExternalFilesDependencies(RootedPath rootedPath, Environment env) throws InterruptedException { 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 236c857be13e4c..80e0a5c21a2270 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; @@ -115,6 +116,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; /** @@ -168,6 +170,7 @@ protected SequencedSkyframeExecutor( CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy, ImmutableList buildFilesByPriority, boolean allowExternalRepositories, + Supplier repoContentsCachePathSupplier, ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile, ActionOnFilesystemErrorCodeLoadingBzlFile actionOnFilesystemErrorCodeLoadingBzlFile, boolean shouldUseRepoDotBazel, @@ -200,6 +203,7 @@ protected SequencedSkyframeExecutor( workspaceInfoFromDiffReceiver, new SequencedRecordingDifferencer(), allowExternalRepositories, + repoContentsCachePathSupplier, globUnderSingleDep, diffCheckNotificationOptions); } @@ -828,6 +832,7 @@ public static final class Builder { private WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver = (ignored1, ignored2) -> {}; private boolean allowExternalRepositories = false; + private Supplier repoContentsCachePathSupplier = () -> null; private Consumer skyframeExecutorConsumerOnInit = skyframeExecutor -> {}; private SkyFunction ignoredSubdirectoriesFunction; private BugReporter bugReporter = BugReporter.defaultInstance(); @@ -866,6 +871,7 @@ public SequencedSkyframeExecutor build() { crossRepositoryLabelViolationStrategy, buildFilesByPriority, allowExternalRepositories, + repoContentsCachePathSupplier, actionOnIOExceptionReadingBuildFile, actionOnFilesystemErrorCodeLoadingBzlFile, shouldUseRepoDotBazel, @@ -946,6 +952,12 @@ public Builder allowExternalRepositories(boolean allowExternalRepositories) { 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 958776668ffabe..a9bbebf1616c4c 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; /** A factory of SkyframeExecutors that returns SequencedSkyframeExecutor. */ public final class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory { @@ -38,6 +40,7 @@ public SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, boolean allowExternalRepositories, + Supplier repoContentsCachePathSupplier, SkyframeExecutor.SkyKeyStateReceiver skyKeyStateReceiver, BugReporter bugReporter) { return BazelSkyframeExecutorConstants.newBazelSkyframeExecutorBuilder() @@ -50,6 +53,7 @@ public SkyframeExecutor create( .setExtraSkyFunctions(extraSkyFunctions) .setSyscallCache(syscallCache) .allowExternalRepositories(allowExternalRepositories) + .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 ae3c1a239e1a6d..eed51785ec7235 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 @@ -688,6 +688,7 @@ protected SkyframeExecutor( @Nullable WorkspaceInfoFromDiffReceiver workspaceInfoFromDiffReceiver, @Nullable RecordingDifferencer recordingDiffer, boolean allowExternalRepositories, + Supplier repoContentsCachePathSupplier, boolean globUnderSingleDep, Optional diffCheckNotificationOptions) { // Strictly speaking, these arguments are not required for initialization, but all current @@ -733,7 +734,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.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile; 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 a2756443f8a25f..7ec2b33ab9299d 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. */ public interface SkyframeExecutorFactory { @@ -50,6 +52,7 @@ SkyframeExecutor create( ImmutableMap extraSkyFunctions, SyscallCache syscallCache, boolean allowExternalRepositories, + 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 7aeb7afa9411b9..746b0ee18f3d9e 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 @@ -288,7 +288,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 2d7b0dc690e088..262dba90c75b97 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 @@ -318,6 +318,7 @@ private void initEvaluator() getExtraSkyFunctions(), SyscallCache.NO_CACHE, /* allowExternalRepositories= */ false, + /* 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 4dbd5ecd53c9dc..25a178d87dd733 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -392,6 +392,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 3ff7ee84a4541e..efaa4e182e6be7 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 @@ -452,6 +489,109 @@ def testReverseDependencyDirection(self): self.assertIn('| @@+repo+bar', stderr) self.assertIn('`-- @@+repo+foo', 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()