From 4674c7dc07aaf61a3fc0385826e1787671060f2d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 18 Nov 2025 02:11:25 -0800 Subject: [PATCH 1/7] [8.8.0] Fix NPE with remote repo contents cache I haven't been able to reproduce this in a test, but this should fix the following crash observed while running `bazel info`: ``` FATAL: bazel crashed due to an internal error. Printing stack trace: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.ExecutorService.shutdownNow()" because "this.materializationExecutor" is null at com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem.afterCommand(RemoteExternalOverlayFileSystem.java:145) at com.google.devtools.build.lib.remote.RemoteModule.afterCommand(RemoteModule.java:1034) at com.google.devtools.build.lib.runtime.BlazeRuntime.afterCommand(BlazeRuntime.java:787) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:807) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:266) at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:608) at com.google.devtools.build.lib.server.GrpcServerImpl.lambda$run$0(GrpcServerImpl.java:679) at io.grpc.Context$1.run(Context.java:566) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) at java.base/java.lang.Thread.run(Unknown Source) ``` Closes #27690. PiperOrigin-RevId: 833722608 Change-Id: I88c485a01e5967657ec3b5529a47639b743b18e6 (cherry picked from commit a7d0e91dce87cde6f9bd8c1e0139db2cd07df4d9) --- .../build/lib/remote/RemoteExternalOverlayFileSystem.java | 5 +++++ 1 file changed, 5 insertions(+) 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 89c7b61f75b256..3425f500a697e6 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 @@ -145,6 +145,11 @@ public void beforeCommand( } public void afterCommand() { + if (cache == null) { + // Not all commands cause beforeCommand to be called, but afterCommand is called + // unconditionally. + return; + } this.cache = null; this.inputPrefetcher = null; this.reporter = null; From 4e4919b1296f22de02a9dc01c5dea9889dec8401 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 18 Nov 2025 20:53:30 -0800 Subject: [PATCH 2/7] [8.8.0] Make remote repo contents cache less spammy Don't print a message when it's successful. Users can always look under `external` to verify which repo came from the cache. Closes #27699. PiperOrigin-RevId: 834096735 Change-Id: I3916fb240218a6b68ecf48417142b998ca281598 (cherry picked from commit 3ca9ce13423393b90564710670b09486955383af) --- .../lib/rules/repository/RepositoryDelegatorFunction.java | 4 ---- 1 file changed, 4 deletions(-) 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 57e40d770200f8..0f7e84d82ff90d 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 @@ -268,10 +268,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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); } From 7545af3373c5b2677d612fa85d9eb5e7f7067c35 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 24 Nov 2025 01:46:40 -0800 Subject: [PATCH 3/7] Fix materialization edge cases in the remote repo contents cache Fixes the creation of empty directories and also contains a speculative fix for the following issue observed during a sequence of real builds: ``` Error in path: Failed to materialize remote repo @@protoc-gen-validate+: [unix_jni.cc:302] /home/ubuntu/.cache/bazel/_bazel_ubuntu/123/external/protoc-gen-validate+/example-workspace/.bazelrc (File exists) ERROR: //:foo :: Error loading option //:foo: error evaluating module extension @@gazelle+//:extensions.bzl%go_deps ``` The mentioned file is a symlink. Closes #27711. PiperOrigin-RevId: 836122472 Change-Id: I8becd8c3640a659d28dc433340db962c18563d9f (cherry picked from commit b27ea05b2ce0864284b38bf3539f8c2fe019ef64) --- .../remote/RemoteExternalOverlayFileSystem.java | 17 +++++++++++------ .../bzlmod/remote_repo_contents_cache_test.py | 9 +++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) 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 3425f500a697e6..1b02d22eb34255 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 @@ -275,6 +275,9 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) var repoPath = externalDirectory.getChild(repo.getName()); var remoteRepo = externalFs.getPath(repoPath); var walkResult = walk(remoteRepo); + for (var directory : walkResult.directories()) { + nativeFs.getPath(directory.asFragment()).createDirectory(); + } var unused = getFromFuture( inputPrefetcher.prefetchFiles( @@ -285,11 +288,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) ActionInputPrefetcher.Priority.CRITICAL, ActionInputPrefetcher.Reason.INPUTS)); // Create symlinks last as some platforms don't allow creating a symlink to a non-existent - // target. + // 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()); - nativeSymlink.getParentDirectory().createDirectoryAndParents(); - nativeSymlink.createSymbolicLink(remoteSymlink.readSymbolicLink()); + FileSystemUtils.ensureSymbolicLink(nativeSymlink, remoteSymlink.readSymbolicLink()); } // After the repo has been copied, atomically materialize the marker file. This ensures that the @@ -302,10 +304,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) markerFileSibling.renameTo(markerFile); } - private record WalkResult(List files, List symlinks) {} + 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<>()); + var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); walk(root, result); return result; } @@ -316,7 +318,10 @@ private static void walk(Path root, WalkResult result) throws IOException { switch (dirent.getType()) { case FILE -> result.files.add(fromChild); case SYMLINK -> result.symlinks.add(fromChild); - case DIRECTORY -> walk(fromChild, result); + case DIRECTORY -> { + result.directories.add(fromChild); + walk(fromChild, result); + } default -> throw new IOException("Unsupported file type: " + dirent); } } 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 57a941e824fa9b..12bf9e29486280 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 @@ -314,6 +314,10 @@ def testAccessFromOtherRepo_read(self): [ 'def _repo_impl(rctx):', ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + # Verify that directories are materialized correctly. + ' rctx.file("subdir/file.txt", "hello")', + ' rctx.file("subdir/empty_dir/.keep")', + ' rctx.delete("subdir/empty_dir/.keep")', ' print("JUST FETCHED")', ' return rctx.repo_metadata(reproducible=True)', 'repo = repository_rule(_repo_impl)', @@ -345,12 +349,17 @@ def testAccessFromOtherRepo_read(self): _, _, 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'))) + self.assertFalse(os.path.exists(os.path.join(repo_dir, 'subdir'))) # 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'))) + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/file.txt'))) + with open(os.path.join(repo_dir, 'subdir/file.txt')) as f: + self.assertEqual(f.read(), 'hello') + self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/empty_dir'))) # Materialized repo is not refetched after a shutdown self.RunBazel(['shutdown']) From 96f4080ffa016652aeb4888e1bad4c0f6694af46 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 28 Nov 2025 06:48:24 -0800 Subject: [PATCH 4/7] [8.7.0] Fix `RemoteExternalOverlayFileSystem#resolveSymbolicLinks` Ensures that the returned `Path` is still in the overlay file system. Also make the error message emitted by `Path#checkSameFileSystem` more informative. This is motivated by and helped discover the above as the fix for the following crash observed when using the remote repo contents cache with an explicit `--sandbox_base`: ``` Caused by: java.lang.IllegalArgumentException: Files are on different filesystems: /dev/shm/bazel-sandbox.b10976335efa519b0184f3091ac8e21f7beefb92142303f9ab2c3341f45a2f28/linux-sandbox/18/execroot/_main/external/c-ares+/configs/ares_build.h (on com.google.devtools.build.lib.unix.UnixFileSystem@5e0a8154), /home/ubuntu/.cache/bazel/_bazel_ubuntu/123/execroot/_main/external/c-ares+/configs/ares_build.h (on com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem@6cd9bfda) at com.google.devtools.build.lib.vfs.Path.checkSameFileSystem(Path.java:964) at com.google.devtools.build.lib.vfs.Path.createSymbolicLink(Path.java:523) at com.google.devtools.build.lib.vfs.Path.createSymbolicLink(Path.java:535) at com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn.copyFile(SymlinkedSandboxedSpawn.java:129) ``` Alternative to https://github.com/bazelbuild/bazel/pull/27721 Closes #27802. PiperOrigin-RevId: 837832265 Change-Id: I3b73167496b011aef66954d59ca3804b4b64996f (cherry picked from commit 8eaf6a90db1d0e95fbade4dd2b7e3b8c57d03629) --- .../RemoteExternalOverlayFileSystem.java | 3 ++- .../google/devtools/build/lib/vfs/Path.java | 3 ++- .../bzlmod/remote_repo_contents_cache_test.py | 21 ++++++++++++++++--- 3 files changed, 22 insertions(+), 5 deletions(-) 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 1b02d22eb34255..39fdaf35d58744 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 @@ -525,7 +525,8 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) @Override public Path resolveSymbolicLinks(PathFragment path) throws IOException { - return fsForPath(path).resolveSymbolicLinks(path); + // Ensure that the return value doesn't leave the overlay file system. + return getPath(fsForPath(path).resolveSymbolicLinks(path).asFragment()); } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index b48d483e7c487a..d62a3da4784456 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java @@ -948,7 +948,8 @@ public void prefetchPackageAsync(int maxDirs) { private void checkSameFileSystem(Path that) { if (this.fileSystem != that.fileSystem) { throw new IllegalArgumentException( - "Files are on different filesystems: " + this + ", " + that); + "Files are on different filesystems: %s (on %s), %s (on %s)" + .formatted(this, this.fileSystem, that, that.fileSystem)); } } } 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 12bf9e29486280..c901e9b216c900 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 @@ -482,7 +482,11 @@ def testUseRepoFileInBuildRule_actionUsesCache(self): with open(self.Path('bazel-bin/main/out.txt')) as f: self.assertEqual(f.read(), 'hello') - def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): + def do_testUseRepoFileInBuildRule_actionDoesNotUseCache( + self, extra_flags=None + ): + if extra_flags is None: + extra_flags = [] self.ScratchFile( 'MODULE.bazel', [ @@ -518,7 +522,7 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): repo_dir = self.RepoDir('my_repo') # First fetch: not cached - _, _, stderr = self.RunBazel(['build', '//main:use_data']) + _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags) 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'))) @@ -528,7 +532,7 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): # After expunging: repo and build action cached self.RunBazel(['clean', '--expunge']) - _, _, stderr = self.RunBazel(['build', '//main:use_data']) + _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags) 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'))) @@ -536,6 +540,17 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): with open(self.Path('bazel-bin/main/out.txt')) as f: self.assertEqual(f.read(), 'hello') + def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): + self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache() + + def testUseRepoFileInBuildRule_actionDoesNotUseCache_withExplicitSandboxBase( + self, + ): + tmpdir = self.ScratchDir('sandbox_base') + self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache( + extra_flags=['--sandbox_base=' + tmpdir] + ) + 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 From d9331e69197ac311263f975648590ad6e65cdf08 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 17 Dec 2025 15:26:39 -0800 Subject: [PATCH 5/7] [8.7.0] Allow more general exceptions in `getConfiguration` Fixes #27981 Fixes the following type of crash and, incidentally, a remote repo contents cache test that resulted in a related crash: ``` FATAL: bazel crashed due to an internal error. Printing stack trace: java.lang.IllegalStateException: Unknown error during configuration creation evaluation at com.google.devtools.build.lib.skyframe.SkyframeExecutor.getConfiguration(SkyframeExecutor.java:2143) at com.google.devtools.build.lib.skyframe.SkyframeExecutor.createConfiguration(SkyframeExecutor.java:1876) at com.google.devtools.build.lib.analysis.BuildView.update(BuildView.java:281) at com.google.devtools.build.lib.buildtool.AnalysisPhaseRunner.runAnalysisPhase(AnalysisPhaseRunner.java:399) at com.google.devtools.build.lib.buildtool.AnalysisPhaseRunner.execute(AnalysisPhaseRunner.java:144) at com.google.devtools.build.lib.buildtool.BuildTool.buildTargetsWithoutMergedAnalysisExecution(BuildTool.java:512) at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:414) at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:907) at com.google.devtools.build.lib.runtime.commands.CqueryCommand.exec(CqueryCommand.java:197) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:783) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:266) at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:608) at com.google.devtools.build.lib.server.GrpcServerImpl.lambda$run$0(GrpcServerImpl.java:679) at io.grpc.Context$1.run(Context.java:566) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) at java.base/java.lang.Thread.run(Unknown Source) Caused by: com.google.devtools.build.lib.skyframe.toolchains.PlatformLookupUtil$InvalidPlatformException: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@@[unknown repo 'toolchains_llvm_boostrapped' requested from @@ (did you mean 'toolchains_llvm_bootstrapped'?)]//platforms': The repository '@@[unknown repo 'toolchains_llvm_boostrapped' requested from @@ (did you mean 'toolchains_llvm_bootstrapped'?)]' could not be resolved: No repository visible as '@toolchains_llvm_boostrapped' from main repository at com.google.devtools.build.lib.analysis.platform.PlatformFunction.compute(PlatformFunction.java:75) at com.google.devtools.build.lib.analysis.platform.PlatformFunction.compute(PlatformFunction.java:43) at com.google.devtools.build.skyframe.ParallelEvaluator.bubbleErrorUp(ParallelEvaluator.java:414) at com.google.devtools.build.skyframe.ParallelEvaluator.waitForCompletionAndConstructResult(ParallelEvaluator.java:207) at com.google.devtools.build.skyframe.ParallelEvaluator.doMutatingEvaluation(ParallelEvaluator.java:173) at com.google.devtools.build.skyframe.ParallelEvaluator.eval(ParallelEvaluator.java:672) at com.google.devtools.build.skyframe.AbstractInMemoryMemoizingEvaluator.evaluate(AbstractInMemoryMemoizingEvaluator.java:182) at com.google.devtools.build.lib.skyframe.SkyframeExecutor.evaluate(SkyframeExecutor.java:4279) at com.google.devtools.build.lib.skyframe.SkyframeExecutor.lambda$evaluateSkyKeys$0(SkyframeExecutor.java:2278) at com.google.devtools.build.lib.concurrent.Uninterruptibles.callUninterruptibly(Uninterruptibles.java:35) at com.google.devtools.build.lib.skyframe.SkyframeExecutor.evaluateSkyKeys(SkyframeExecutor.java:2274) at com.google.devtools.build.lib.skyframe.SkyframeExecutor.getConfiguration(SkyframeExecutor.java:2126) ... 16 more ``` Closes #28004. PiperOrigin-RevId: 845941915 Change-Id: I6ead8dd1662efe90f529a6e21041a225882415dc (cherry picked from commit d6dc63124242e697b6896777db0ca83c103643b5) --- .../build/lib/skyframe/SkyframeExecutor.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) 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 1d21fe069c48fe..f57e51c8af78b0 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 @@ -1920,19 +1920,17 @@ public BuildConfigurationValue getConfiguration( Map.Entry firstError = Iterables.get(evalResult.errorMap().entrySet(), 0); ErrorInfo error = firstError.getValue(); Throwable e = error.getException(); - // Wrap loading failed exceptions - if (e != null && e instanceof NoSuchThingException noSuchThingException) { - e = new InvalidConfigurationException(noSuchThingException.getDetailedExitCode(), e); + if (e instanceof InvalidConfigurationException invalidConfigurationException) { + throw invalidConfigurationException; + } else if (e instanceof DetailedException detailedException) { + throw new InvalidConfigurationException(detailedException.getDetailedExitCode(), e); } else if (e == null && !error.getCycleInfo().isEmpty()) { cyclesReporter.reportCycles(error.getCycleInfo(), firstError.getKey(), eventHandler); - e = - new InvalidConfigurationException( + throw new InvalidConfigurationException( "cannot load build configuration because of this cycle", Code.CYCLE); } - if (e != null) { - Throwables.throwIfInstanceOf(e, InvalidConfigurationException.class); - } - throw new IllegalStateException("Unknown error during configuration creation evaluation", e); + throw new IllegalStateException( + "Unknown error during configuration creation evaluation", e); } // Prepare and return the results. From ac8b751621924bf0426e4bc1b02884de9c23b0ae Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 15 Dec 2025 05:05:09 -0800 Subject: [PATCH 6/7] Preserve order of recorded inputs Recorded inputs for repo rules and module extensions are no longer separated by type, but recorded in the order they are encountered. This is necessary preparatory work to avoid Skyframe cycles that could otherwise occur while checking cache, lockfile or marker file entries for up-to-dateness. This requires a breaking change to the lockfile structure, but has the positive side effect that each recorded input is now represented as a single line in the lockfile and thus more compact. Along the way, the error message for an out-of-date lockfile entry with `--lockfile_mode=errors` are improved and now include the precise reason for the invalidation. For repository rules, the repo mapping entries recorded while loading the repo rule definition are now included in the predeclared inputs hash, which makes cache entries more specific and avoids unnecessary Skyframe lookups. Work towards #27517 Closes #27603. PiperOrigin-RevId: 844721202 Change-Id: Id063a88508591086b07bd89a822cf22b2d90610d (cherry picked from commit 41ccfefb88e9d816453eb42e549895b179c64061) --- .../lib/bazel/bzlmod/BazelLockFileValue.java | 2 +- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 119 ++------- .../bazel/bzlmod/InnateRunnableExtension.java | 15 +- .../bazel/bzlmod/LockFileModuleExtension.java | 28 +-- .../bazel/bzlmod/ModuleExtensionContext.java | 12 +- .../bzlmod/RegularRunnableExtension.java | 33 +-- .../lib/bazel/bzlmod/RunnableExtension.java | 20 +- .../bzlmod/SingleExtensionEvalFunction.java | 121 ++-------- .../cache/LocalRepoContentsCache.java | 2 +- .../starlark/StarlarkBaseExternalContext.java | 73 +++--- .../starlark/StarlarkRepositoryContext.java | 9 +- .../starlark/StarlarkRepositoryFunction.java | 19 +- .../starlark/StarlarkRepositoryModule.java | 4 +- .../devtools/build/lib/cmdline/Label.java | 29 ++- .../rules/repository/RepoRecordedInput.java | 105 +++++--- .../RepositoryDelegatorFunction.java | 75 ++---- .../rules/repository/RepositoryFunction.java | 7 +- .../build/lib/skyframe/BzlLoadFunction.java | 4 +- .../devtools/build/lib/bazel/bzlmod/BUILD | 2 + .../bazel/bzlmod/BazelLockFileModuleTest.java | 10 +- .../bzlmod/ModuleExtensionResolutionTest.java | 8 +- .../bazel/bzlmod/StarlarkBazelModuleTest.java | 4 +- .../build/lib/bazel/repository/starlark/BUILD | 1 + .../StarlarkRepositoryContextTest.java | 11 +- .../devtools/build/lib/cmdline/LabelTest.java | 3 +- ...onTest.java => RepoRecordedInputTest.java} | 13 +- .../py/bazel/bzlmod/bazel_lockfile_test.py | 24 +- src/test/tools/bzlmod/MODULE.bazel.lock | 227 ++++-------------- 28 files changed, 351 insertions(+), 629 deletions(-) rename src/test/java/com/google/devtools/build/lib/rules/repository/{RepositoryFunctionTest.java => RepoRecordedInputTest.java} (89%) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 398df8266ffe65..b96e3eaa1f2850 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -47,7 +47,7 @@ public abstract class BazelLockFileValue implements SkyValue { // https://cs.opensource.google/bazel/bazel/+/release-7.3.0:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java;l=120-127;drc=5f5355b75c7c93fba1e15f6658f308953f4baf51 // While this hack exists on 7.x, lockfile version increments should be done 2 at a time (i.e. // keep this number even). - public static final int LOCK_FILE_VERSION = 24; + public static final int LOCK_FILE_VERSION = 26; /** A valid empty lockfile. */ public static final BazelLockFileValue EMPTY_LOCKFILE = builder().build(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index f509b01a194cfc..433124f1061085 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -20,11 +20,12 @@ import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SORTED_MAP; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.NeverUpToDateRepoRecordedInput.PARSE_FAILURE; +import static com.google.devtools.build.lib.util.StringEncoding.internalToUnicode; +import static com.google.devtools.build.lib.util.StringEncoding.unicodeToInternal; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Table; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.devtools.build.lib.bazel.repository.cache.DownloadCache; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; @@ -293,113 +294,34 @@ public Optional read(JsonReader jsonReader) throws IOException { } } - /** - * Converts Guava tables into a JSON array of 3-tuples (one per cell). Each 3-tuple is a JSON - * array itself (rowKey, columnKey, value). For example, a JSON snippet could be: {@code [ - * ["row1", "col1", "value1"], ["row2", "col2", "value2"], ... ]} - */ - public static final TypeAdapterFactory IMMUTABLE_TABLE = - new TypeAdapterFactory() { - @Nullable - @Override - @SuppressWarnings("unchecked") - public TypeAdapter create(Gson gson, TypeToken typeToken) { - if (typeToken.getRawType() != ImmutableTable.class) { - return null; - } - Type type = typeToken.getType(); - if (!(type instanceof ParameterizedType)) { - return null; - } - Type[] typeArgs = ((ParameterizedType) typeToken.getType()).getActualTypeArguments(); - if (typeArgs.length != 3) { - return null; - } - var rowTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[0])); - var colTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[1])); - var valTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[2])); - if (rowTypeAdapter == null || colTypeAdapter == null || valTypeAdapter == null) { - return null; - } - return (TypeAdapter) - new TypeAdapter>() { - @Override - public void write(JsonWriter jsonWriter, ImmutableTable t) - throws IOException { - jsonWriter.beginArray(); - for (Table.Cell cell : t.cellSet()) { - jsonWriter.beginArray(); - rowTypeAdapter.write(jsonWriter, cell.getRowKey()); - colTypeAdapter.write(jsonWriter, cell.getColumnKey()); - valTypeAdapter.write(jsonWriter, cell.getValue()); - jsonWriter.endArray(); - } - jsonWriter.endArray(); - } - - @Override - public ImmutableTable read(JsonReader jsonReader) - throws IOException { - var builder = ImmutableTable.builder(); - jsonReader.beginArray(); - while (jsonReader.peek() != JsonToken.END_ARRAY) { - jsonReader.beginArray(); - builder.put( - rowTypeAdapter.read(jsonReader), - colTypeAdapter.read(jsonReader), - valTypeAdapter.read(jsonReader)); - jsonReader.endArray(); - } - jsonReader.endArray(); - return builder.buildOrThrow(); - } - }; - } - }; - - private static final TypeAdapter REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER = + // This is needed because Bazel uses a custom String encoding internally, see StringEncoding for + // details. + public static final TypeAdapter STRING_ADAPTER = new TypeAdapter<>() { @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.File value) throws IOException { - jsonWriter.value(value.toStringInternal()); + public void write(JsonWriter jsonWriter, String s) throws IOException { + jsonWriter.value(internalToUnicode(s)); } @Override - public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.File) - RepoRecordedInput.File.PARSER.parse(jsonReader.nextString()); + public String read(JsonReader jsonReader) throws IOException { + return unicodeToInternal(jsonReader.nextString()); } }; - private static final TypeAdapter - REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER = - new TypeAdapter<>() { - @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.Dirents value) - throws IOException { - jsonWriter.value(value.toStringInternal()); - } - - @Override - public RepoRecordedInput.Dirents read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.Dirents) - RepoRecordedInput.Dirents.PARSER.parse(jsonReader.nextString()); - } - }; - - private static final TypeAdapter - REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER = + private static final TypeAdapter + REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER = new TypeAdapter<>() { @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.EnvVar value) + public void write(JsonWriter jsonWriter, RepoRecordedInput.WithValue value) throws IOException { - jsonWriter.value(value.toStringInternal()); + jsonWriter.value(internalToUnicode(value.toString())); } @Override - public RepoRecordedInput.EnvVar read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.EnvVar) - RepoRecordedInput.EnvVar.PARSER.parse(jsonReader.nextString()); + public RepoRecordedInput.WithValue read(JsonReader jsonReader) throws IOException { + return RepoRecordedInput.WithValue.parse(unicodeToInternal(jsonReader.nextString())) + .orElseGet(() -> new RepoRecordedInput.WithValue(PARSE_FAILURE, "")); } }; @@ -475,7 +397,7 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapterFactory(IMMUTABLE_BIMAP) .registerTypeAdapterFactory(IMMUTABLE_SET) .registerTypeAdapterFactory(OPTIONAL) - .registerTypeAdapterFactory(IMMUTABLE_TABLE) + .registerTypeAdapter(String.class, STRING_ADAPTER) .registerTypeAdapter(Label.class, LABEL_TYPE_ADAPTER) .registerTypeAdapter(RepoRuleId.class, REPO_RULE_ID_TYPE_ADAPTER) .registerTypeAdapter(RepositoryName.class, REPOSITORY_NAME_TYPE_ADAPTER) @@ -487,11 +409,8 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapter(ModuleExtensionId.IsolationKey.class, ISOLATION_KEY_TYPE_ADAPTER) .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter()) .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER) - .registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER) - .registerTypeAdapter( - RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER) .registerTypeAdapter( - RepoRecordedInput.EnvVar.class, REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER); + RepoRecordedInput.WithValue.class, REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER); } private GsonTypeAdapterUtil() {} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java index 913d3d24afe199..6599336192b29c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java @@ -21,8 +21,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -39,7 +37,6 @@ import com.google.devtools.build.lib.skyframe.BzlLoadValue; import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.util.Map.Entry; -import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -150,11 +147,6 @@ public byte[] getBzlTransitiveDigest() { return loadedBzl.getTransitiveDigest(); } - @Override - public ImmutableMap> getStaticEnvVars() { - return ImmutableMap.of(); - } - @Override public RunModuleExtensionResult run( Environment env, @@ -237,11 +229,8 @@ public RunModuleExtensionResult run( attributesValue)); } return new RunModuleExtensionResult( - ImmutableSortedMap.of(), - ImmutableSortedMap.of(), - ImmutableSortedMap.of(), + ImmutableList.of(), generatedRepoSpecs.buildOrThrow(), - ModuleExtensionMetadata.REPRODUCIBLE, - ImmutableTable.of()); + ModuleExtensionMetadata.REPRODUCIBLE); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 8718920f4b14ab..6a0a622c7f4fc9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -15,10 +15,8 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; -import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; @@ -35,8 +33,7 @@ public abstract class LockFileModuleExtension { public static Builder builder() { return new AutoValue_LockFileModuleExtension.Builder() - .setModuleExtensionMetadata(Optional.empty()) - .setRecordedRepoMappingEntries(ImmutableTable.of()); + .setModuleExtensionMetadata(Optional.empty()); } @SuppressWarnings("mutable") @@ -45,19 +42,12 @@ public static Builder builder() { @SuppressWarnings("mutable") public abstract byte[] getUsagesDigest(); - public abstract ImmutableSortedMap getRecordedFileInputs(); - - public abstract ImmutableSortedMap getRecordedDirentsInputs(); - - public abstract ImmutableSortedMap> getEnvVariables(); + public abstract ImmutableList getRecordedInputs(); public abstract ImmutableMap getGeneratedRepoSpecs(); public abstract Optional getModuleExtensionMetadata(); - public abstract ImmutableTable - getRecordedRepoMappingEntries(); - public boolean isReproducible() { return getModuleExtensionMetadata() .map(LockfileModuleExtensionMetadata::getReproducible) @@ -72,23 +62,13 @@ public abstract static class Builder { public abstract Builder setUsagesDigest(byte[] digest); - public abstract Builder setRecordedFileInputs( - ImmutableSortedMap value); - - public abstract Builder setRecordedDirentsInputs( - ImmutableSortedMap value); - - public abstract Builder setEnvVariables( - ImmutableSortedMap> value); + public abstract Builder setRecordedInputs(ImmutableList value); public abstract Builder setGeneratedRepoSpecs(ImmutableMap value); public abstract Builder setModuleExtensionMetadata( Optional value); - public abstract Builder setRecordedRepoMappingEntries( - ImmutableTable value); - public abstract LockFileModuleExtension build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index d2135a6ba4aa82..5c7b8de8a1d6c7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -15,15 +15,19 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableTable; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; @@ -65,7 +69,9 @@ protected ModuleExtensionContext( ModuleExtensionId extensionId, StarlarkList modules, Facts facts, - boolean rootModuleHasNonDevDependency) { + boolean rootModuleHasNonDevDependency, + ImmutableMap> staticEnvVars, + ImmutableTable staticRepoMappingEntries) { super( workingDirectory, directories, @@ -83,6 +89,10 @@ protected ModuleExtensionContext( this.modules = modules; this.facts = facts; this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency; + // Record inputs to the extension that are known prior to evaluation. + RepoRecordedInput.EnvVar.wrap(staticEnvVars) + .forEach((input, value) -> recordInput(input, value.orElse(null))); + repoMappingRecorder.record(staticRepoMappingEntries); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java index ac6deab5150f4b..af6d7392b4a16b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java @@ -205,11 +205,6 @@ public ModuleExtensionEvalFactors getEvalFactors() { extension.archDependent() ? OS_ARCH.value() : ""); } - @Override - public ImmutableMap> getStaticEnvVars() { - return staticEnvVars; - } - @Override public byte[] getBzlTransitiveDigest() { return BazelModuleContext.of(bzlLoadValue.getModule()).bzlTransitiveDigest(); @@ -274,13 +269,10 @@ private RunModuleExtensionResult runInternal( directories, env.getListener()); ModuleExtensionMetadata moduleExtensionMetadata; - var repoMappingRecorder = new Label.RepoMappingRecorder(); - repoMappingRecorder.mergeEntries(bzlLoadValue.getRecordedRepoMappings()); try (Mutability mu = Mutability.create("module extension", usagesValue.getExtensionUniqueName()); ModuleExtensionContext moduleContext = - createContext( - env, usagesValue, starlarkSemantics, extensionId, repoMappingRecorder, facts)) { + createContext(env, usagesValue, starlarkSemantics, extensionId, facts, bzlLoadValue)) { StarlarkThread thread = StarlarkThread.create( mu, @@ -289,9 +281,7 @@ private RunModuleExtensionResult runInternal( SymbolGenerator.create(extensionId)); thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); threadContext.storeInThread(thread); - // This is used by the `Label()` constructor in Starlark, to record any attempts to resolve - // apparent repo names to canonical repo names. See #20721 for why this is necessary. - thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder); + moduleContext.storeRepoMappingRecorderInThread(thread); try (SilentCloseable c = Profiler.instance() .profile(ProfilerTask.BZLMOD, () -> "evaluate module extension: " + extensionId)) { @@ -317,12 +307,9 @@ private RunModuleExtensionResult runInternal( moduleContext.markSuccessful(); env.getListener().post(ModuleExtensionEvaluationProgress.finished(extensionId)); return new RunModuleExtensionResult( - moduleContext.getRecordedFileInputs(), - moduleContext.getRecordedDirentsInputs(), - moduleContext.getRecordedEnvVarInputs(), + moduleContext.getRecordedInputs(), threadContext.createRepos(starlarkSemantics), - moduleExtensionMetadata, - repoMappingRecorder.recordedEntries()); + moduleExtensionMetadata); } catch (EvalException e) { env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack())); throw ExternalDepsException.withMessage( @@ -340,9 +327,11 @@ private ModuleExtensionContext createContext( SingleExtensionUsagesValue usagesValue, StarlarkSemantics starlarkSemantics, ModuleExtensionId extensionId, - Label.RepoMappingRecorder repoMappingRecorder, - Facts facts) + Facts facts, + BzlLoadValue bzlLoadValue) throws ExternalDepsException { + var staticRepoMappingRecorder = new Label.SimpleRepoMappingRecorder(); + staticRepoMappingRecorder.record(bzlLoadValue.getRecordedRepoMappings()); Path workingDirectory = directories .getOutputBase() @@ -357,7 +346,7 @@ private ModuleExtensionContext createContext( extension, usagesValue.getRepoMappings().get(moduleKey), usagesValue.getExtensionUsages().get(moduleKey), - repoMappingRecorder)); + staticRepoMappingRecorder)); } ModuleExtensionUsage rootUsage = usagesValue.getExtensionUsages().get(ModuleKey.ROOT); boolean rootModuleHasNonDevDependency = @@ -376,6 +365,8 @@ private ModuleExtensionContext createContext( extensionId, StarlarkList.immutableCopyOf(modules), facts, - rootModuleHasNonDevDependency); + rootModuleHasNonDevDependency, + staticEnvVars, + staticRepoMappingRecorder.recordedEntries()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java index 9065194928ef2c..b3a2f7a499e318 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java @@ -15,14 +15,11 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; import com.google.devtools.build.lib.cmdline.RepositoryMapping; -import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.skyframe.SkyFunction.Environment; -import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -33,17 +30,15 @@ * *

The general idiom is to "load" such a {@link RunnableExtension} object by getting as much * information about it as needed to determine whether it can be reused from the lockfile (hence - * methods such as {@link #getEvalFactors()}, {@link #getBzlTransitiveDigest()}, {@link - * #getStaticEnvVars()}). Then the {@link #run} method can be called if it's determined that we - * can't reuse the cached results in the lockfile and have to re-run this extension. + * methods such as {@link #getEvalFactors()} and {@link #getBzlTransitiveDigest()}). Then the {@link + * #run} method can be called if it's determined that we can't reuse the cached results in the + * lockfile and have to re-run this extension. */ interface RunnableExtension { ModuleExtensionEvalFactors getEvalFactors(); byte[] getBzlTransitiveDigest(); - ImmutableMap> getStaticEnvVars(); - /** Runs the extension. Returns null if a Skyframe restart is required. */ @Nullable RunModuleExtensionResult run( @@ -57,10 +52,7 @@ RunModuleExtensionResult run( /* Holds the result data from running a module extension */ record RunModuleExtensionResult( - ImmutableSortedMap recordedFileInputs, - ImmutableSortedMap recordedDirentsInputs, - ImmutableSortedMap> recordedEnvVarInputs, + ImmutableList recordedInputs, ImmutableMap generatedRepoSpecs, - ModuleExtensionMetadata moduleExtensionMetadata, - ImmutableTable recordedRepoMappingEntries) {} + ModuleExtensionMetadata moduleExtensionMetadata) {} } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 052240aa2288cd..c83afc25132cf3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -16,15 +16,10 @@ package com.google.devtools.build.lib.bazel.bzlmod; import static com.google.common.collect.ImmutableBiMap.toImmutableBiMap; -import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.stream.Collectors.joining; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Maps; -import com.google.common.collect.Table; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.RunnableExtension.RunModuleExtensionResult; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; @@ -45,8 +40,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Function; @@ -220,9 +215,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (!lockfileMode.equals(LockfileMode.OFF)) { var nonVisibleRepoNames = - moduleExtensionResult.recordedRepoMappingEntries().values().stream() - .filter(repoName -> !repoName.isVisible()) - .map(RepositoryName::toString) + moduleExtensionResult.recordedInputs().stream() + .filter( + inputAndValue -> + inputAndValue.input() instanceof RepoRecordedInput.RecordedRepoMapping + && inputAndValue.value() == null) + .map(entry -> (RepoRecordedInput.RecordedRepoMapping) entry.input()) + .map(RepoRecordedInput.RecordedRepoMapping::apparentName) + .map(apparentName -> "@" + apparentName) .collect(joining(", ")); if (!nonVisibleRepoNames.isEmpty()) { env.getListener() @@ -252,7 +252,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // rollback), which would cause false-positive validation errors. if (lockfileMode.equals(LockfileMode.ERROR) && !newFacts.equals(workspaceLockfileFacts)) { String reason = - "The extension '%s' has changed its facts: %s != %s" + "the extension '%s' has changed its facts: %s != %s" .formatted( extensionId, Starlark.repr(newFacts.value()), @@ -269,15 +269,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) // result is taken from the lockfile, we can already populate the lockfile info. This is // necessary to prevent the extension from rerunning when only the imports change. if (lockfileMode == LockfileMode.UPDATE || lockfileMode == LockfileMode.REFRESH) { - var envVariables = - ImmutableMap.>builder() - // The environment variable dependencies statically declared via the 'environ' - // attribute. - .putAll(RepoRecordedInput.EnvVar.wrap(extension.getStaticEnvVars())) - // The environment variable dependencies dynamically declared via the 'getenv' method. - .putAll(moduleExtensionResult.recordedEnvVarInputs()) - .buildKeepingLast(); - lockFileInfo = Optional.of( new LockFileModuleExtension.WithFactors( @@ -287,13 +278,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) .setUsagesDigest( SingleExtensionUsagesValue.hashForEvaluation( GsonTypeAdapterUtil.SINGLE_EXTENSION_USAGES_VALUE_GSON, usagesValue)) - .setRecordedFileInputs(moduleExtensionResult.recordedFileInputs()) - .setRecordedDirentsInputs(moduleExtensionResult.recordedDirentsInputs()) - .setEnvVariables(ImmutableSortedMap.copyOf(envVariables)) + .setRecordedInputs(moduleExtensionResult.recordedInputs()) .setGeneratedRepoSpecs(generatedRepoSpecs) .setModuleExtensionMetadata(lockfileModuleExtensionMetadata) - .setRecordedRepoMappingEntries( - moduleExtensionResult.recordedRepoMappingEntries()) .build())); } else { lockFileInfo = Optional.empty(); @@ -335,43 +322,23 @@ private SingleExtensionValue tryGettingValueFromLockFile( if (!Arrays.equals( extension.getBzlTransitiveDigest(), lockedExtension.getBzlTransitiveDigest())) { diffRecorder.record( - "The implementation of the extension '" + "the implementation of the extension '" + extensionId + "' or one of its transitive .bzl files has changed"); } - if (didRecordedInputsChange( - env, - directories, - // didRecordedInputsChange expects possibly null String values. - Maps.transformValues(lockedExtension.getEnvVariables(), v -> v.orElse(null)))) { - diffRecorder.record( - "The environment variables the extension '" - + extensionId - + "' depends on (or their values) have changed"); - } // Check extension data in lockfile is still valid, disregarding usage information that is not // relevant for the evaluation of the extension. if (!Arrays.equals( SingleExtensionUsagesValue.hashForEvaluation( GsonTypeAdapterUtil.SINGLE_EXTENSION_USAGES_VALUE_GSON, usagesValue), lockedExtension.getUsagesDigest())) { - diffRecorder.record("The usages of the extension '" + extensionId + "' have changed"); - } - if (didRepoMappingsChange(env, lockedExtension.getRecordedRepoMappingEntries())) { - diffRecorder.record( - "The repo mappings of certain repos used by the extension '" - + extensionId - + "' have changed"); + diffRecorder.record("the usages of the extension '" + extensionId + "' have changed"); } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedFileInputs())) { + Optional reason = + didRecordedInputsChange(env, directories, lockedExtension.getRecordedInputs()); + if (reason.isPresent()) { diffRecorder.record( - "One or more files the extension '" + extensionId + "' is using have changed"); - } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedDirentsInputs())) { - diffRecorder.record( - "One or more directory listings watched by the extension '" - + extensionId - + "' have changed"); + "an input to the extension '" + extensionId + "' changed: " + reason.get()); } } catch (DiffFoundEarlyExitException ignored) { // ignored @@ -423,63 +390,17 @@ public String getRecordedDiffMessages() { } } - private static boolean didRepoMappingsChange( - Environment env, ImmutableTable recordedRepoMappings) - throws InterruptedException, NeedsSkyframeRestartException { - // Request repo mappings for any 'source repos' in the recorded mapping entries. - // Note specially that the main repo needs to be treated differently: if any .bzl file from the - // main repo was used for module extension eval, it _has_ to be before WORKSPACE is evaluated - // (see relevant code in BzlLoadFunction#getRepositoryMapping), so we only request the main repo - // mapping _without_ WORKSPACE repos. See #20942 for more information. - SkyframeLookupResult result = - env.getValuesAndExceptions( - recordedRepoMappings.rowKeySet().stream() - .map( - repoName -> - repoName.isMain() - ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS - : RepositoryMappingValue.key(repoName)) - .collect(toImmutableSet())); - if (env.valuesMissing()) { - // This likely means that one of the 'source repos' in the recorded mapping entries is no - // longer there. - throw new NeedsSkyframeRestartException(); - } - for (Table.Cell cell : recordedRepoMappings.cellSet()) { - RepositoryMappingValue repoMappingValue = - (RepositoryMappingValue) - result.get( - cell.getRowKey().isMain() - ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS - : RepositoryMappingValue.key(cell.getRowKey())); - if (repoMappingValue == null) { - throw new NeedsSkyframeRestartException(); - } - // Very importantly, `repoMappingValue` here could be for a repo that's no longer existent in - // the dep graph. See - // bazel_lockfile_test.testExtensionRepoMappingChange_sourceRepoNoLongerExistent for a test - // case. - if (repoMappingValue.equals(RepositoryMappingValue.NOT_FOUND_VALUE) - || !cell.getValue() - .equals(repoMappingValue.repositoryMapping().get(cell.getColumnKey()))) { - // Wee woo wee woo -- diff detected! - return true; - } - } - return false; - } - - private static boolean didRecordedInputsChange( + private static Optional didRecordedInputsChange( Environment env, BlazeDirectories directories, - Map recordedInputs) + List recordedInputs) throws InterruptedException, NeedsSkyframeRestartException { Optional outdated = RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs); if (env.valuesMissing()) { throw new NeedsSkyframeRestartException(); } - return outdated.isPresent(); + return outdated; } private SingleExtensionValue createSingleExtensionValue( @@ -533,7 +454,7 @@ private static SingleExtensionEvalFunctionException createOutdatedLockfileExcept return new SingleExtensionEvalFunctionException( ExternalDepsException.withMessage( Code.BAD_LOCKFILE, - "MODULE.bazel.lock is no longer up-to-date because: %s. Please run `bazel mod deps" + "MODULE.bazel.lock is no longer up-to-date because %s. Please run `bazel mod deps" + " --lockfile_mode=update` to update your lockfile.", reason)); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java index a53211fd4b8cec..89af2cde2a1f14 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java @@ -158,7 +158,7 @@ private Path ensureTrashDir() throws IOException { */ public Path moveToCache( Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash) - throws IOException, InterruptedException { + throws IOException { Preconditions.checkState(path != null); Path entryDir = path.getRelative(predeclaredInputHash); 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 17f3d25a6fbad7..5cf3e3069fcf0d 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 @@ -14,15 +14,18 @@ package com.google.devtools.build.lib.bazel.repository.starlark; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.base.Ascii; -import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.Futures; @@ -52,14 +55,12 @@ 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; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; -import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; import com.google.devtools.build.lib.unsafe.StringUnsafe; import com.google.devtools.build.lib.util.OsUtils; import com.google.devtools.build.lib.util.io.OutErr; @@ -85,12 +86,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; -import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -167,9 +167,8 @@ private interface AsyncTask extends SilentCloseable { @Nullable private final ProcessWrapper processWrapper; protected final StarlarkSemantics starlarkSemantics; protected final String identifyingStringForLogging; - private final HashMap recordedFileInputs = new HashMap<>(); - private final HashMap recordedDirentsInputs = new HashMap<>(); - private final HashSet accumulatedEnvKeys = new HashSet<>(); + protected final Label.RepoMappingRecorder repoMappingRecorder; + private final LinkedHashMap recordedInputs = new LinkedHashMap<>(); private final RepositoryRemoteExecutor remoteExecutor; private final List asyncTasks; private final boolean allowWatchingPathsOutsideWorkspace; @@ -209,6 +208,13 @@ protected StarlarkBaseExternalContext( Thread.ofVirtual() .name("downloads[" + identifyingStringForLogging + "]-", 0) .factory()); + // This is used by the `Label()` constructor in Starlark, to record any attempts to resolve + // apparent repo names to canonical repo names. See #20721 for why this is necessary. + this.repoMappingRecorder = + (fromRepo, apparentRepoName, canonicalRepoName) -> + recordInput( + new RepoRecordedInput.RecordedRepoMapping(fromRepo, apparentRepoName), + canonicalRepoName.isVisible() ? canonicalRepoName.getName() : null); } /** @@ -241,6 +247,19 @@ public final void close() throws EvalException, IOException { } } + public void storeRepoMappingRecorderInThread(StarlarkThread thread) { + repoMappingRecorder.storeInThread(thread); + } + + protected void recordInput(RepoRecordedInput input, @Nullable String value) { + if (recordedInputs.containsKey(input) && !Objects.equals(recordedInputs.get(input), value)) { + throw new IllegalStateException( + "Conflicting values recorded for input %s: '%s' vs. '%s'" + .formatted(input, recordedInputs.get(input), value)); + } + recordedInputs.put(input, value); + } + private boolean cancelPendingAsyncTasks() { boolean hadPendingItems = false; for (AsyncTask task : asyncTasks) { @@ -270,21 +289,11 @@ private final void registerAsyncTask(AsyncTask task) { @ForOverride protected abstract boolean shouldDeleteWorkingDirectoryOnClose(boolean successful); - /** Returns the file digests used by this context object so far. */ - public ImmutableSortedMap getRecordedFileInputs() { - return ImmutableSortedMap.copyOf(recordedFileInputs); - } - - public ImmutableSortedMap getRecordedDirentsInputs() { - return ImmutableSortedMap.copyOf(recordedDirentsInputs); - } - - public ImmutableSortedMap> getRecordedEnvVarInputs() - throws InterruptedException { - // getEnvVarValues doesn't return null since the Skyframe dependencies have already been - // established by getenv calls. - return RepoRecordedInput.EnvVar.wrap( - RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys)); + /** Returns all recorded inputs in the order they were recorded. */ + public ImmutableList getRecordedInputs() { + return recordedInputs.entrySet().stream() + .map(e -> new RepoRecordedInput.WithValue(e.getKey(), e.getValue())) + .collect(toImmutableList()); } protected void checkInOutputDirectory(String operation, StarlarkPath path) @@ -1480,16 +1489,17 @@ private static T nullIfNone(Object object, Class type) { @Nullable public String getEnvironmentValue(String name, Object defaultValue) throws InterruptedException, NeedsSkyframeRestartException { - // Must look up via AEF, rather than solely copy from `this.repoEnvVariables`, in order to + // Must look up via Skyframe, rather than solely copy from `this.repoEnvVariables`, in order to // establish a SkyKey dependency relationship. - if (env.getValue(ActionEnvironmentFunction.key(name)) == null) { - throw new NeedsSkyframeRestartException(); + var nameAndValue = RepositoryFunction.getEnvVarValues(env, ImmutableSet.of(name)); + if (nameAndValue == null) { + return null; } - // However, to account for --repo_env we take the value from `this.repoEnvVariables`. // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 . + var entry = Iterables.getOnlyElement(RepoRecordedInput.EnvVar.wrap(nameAndValue).entrySet()); String envVarValue = repoEnvVariables.get(name); - accumulatedEnvKeys.add(name); + recordInput(entry.getKey(), envVarValue); return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class); } @@ -1659,7 +1669,7 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) throw new NeedsSkyframeRestartException(); } - recordedFileInputs.put( + recordInput( recordedInput, RepoRecordedInput.File.fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); } catch (IOException e) { @@ -1678,8 +1688,7 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) throw new NeedsSkyframeRestartException(); } try { - recordedDirentsInputs.put( - recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); + recordInput(recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -1813,7 +1822,7 @@ private StarlarkExecutionResult executeRemote( boolean quiet, String workingDirectory) throws EvalException, InterruptedException { - Preconditions.checkState(canExecuteRemote()); + checkState(canExecuteRemote()); ImmutableSortedMap.Builder inputsBuilder = ImmutableSortedMap.naturalOrder(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 59b79202b21653..751d8305e94a58 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -19,7 +19,6 @@ import com.github.difflib.patch.PatchFailedException; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; @@ -54,7 +53,6 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.file.InvalidPathException; -import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -85,7 +83,6 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { private final StructImpl attrObject; private final IgnoredSubdirectories ignoredSubdirectories; private final SyscallCache syscallCache; - private final HashMap recordedDirTreeInputs = new HashMap<>(); /** * Create a new context (repository_ctx) object for a Starlark repository rule ({@code rule} @@ -142,10 +139,6 @@ protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) { return !successful; } - public ImmutableSortedMap getRecordedDirTreeInputs() { - return ImmutableSortedMap.copyOf(recordedDirTreeInputs); - } - @StarlarkMethod( name = "name", structField = true, @@ -602,7 +595,7 @@ public void watchTree(Object path) throw new NeedsSkyframeRestartException(); } - recordedDirTreeInputs.put(recordedInput, digestValue.hexDigest()); + recordInput(recordedInput, digestValue.hexDigest()); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index f1edc2b714d89e..c804ee6c9dc6c0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -271,13 +271,13 @@ private FetchResult fetchInternal( "repository " + ((RepositoryName) key.argument()).getDisplayForm(mainRepoMapping), SymbolGenerator.create(key)); thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); - var repoMappingRecorder = new Label.RepoMappingRecorder(); - // For repos defined in Bzlmod, record any used repo mappings in the marker file. + var repoMappingRecorder = new Label.SimpleRepoMappingRecorder(); + // For repos defined in Bzlmod, record any repo mappings used by the rule's implementation in + // the marker file. The repo mappings recorded while loading the repo rule definition are + // instead folded into the predeclared input hash (see DigestWriter#computePredeclaredInputHash). // Repos defined in WORKSPACE are impossible to verify given the chunked loading (we'd have to // record which chunk the repo mapping was used in, and ain't nobody got time for that). if (!isWorkspaceRepo(rule)) { - repoMappingRecorder.mergeEntries( - rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries()); thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder); } @@ -328,13 +328,10 @@ private FetchResult fetchInternal( } // Modify marker data to include the files/dirents/env vars used by the rule's implementation - // function. - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs()); - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirTreeInputs()); - recordedInputValues.putAll( - Maps.transformValues( - starlarkRepositoryContext.getRecordedEnvVarInputs(), v -> v.orElse(null))); + // function, in the order they were recorded. + for (RepoRecordedInput.WithValue wv : starlarkRepositoryContext.getRecordedInputs()) { + recordedInputValues.put(wv.input(), wv.value()); + } for (Table.Cell repoMappings : repoMappingRecorder.recordedEntries().cellSet()) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 8a89f9b6999cc0..78cf2c99720123 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -119,8 +119,8 @@ public StarlarkCallable repositoryRule( builder.setRuleDefinitionEnvironmentLabelAndDigest( moduleContext.label(), moduleContext.bzlTransitiveDigest()); } - Label.RepoMappingRecorder repoMappingRecorder = - thread.getThreadLocal(Label.RepoMappingRecorder.class); + Label.SimpleRepoMappingRecorder repoMappingRecorder = + (Label.SimpleRepoMappingRecorder) thread.getThreadLocal(Label.RepoMappingRecorder.class); if (repoMappingRecorder != null) { builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries()); } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index bdf5c89104cc36..d6922efa3f3a3b 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -232,7 +232,7 @@ public static Label parseWithPackageContext( Parts parts = Parts.parse(raw); Label parsed = parseWithPackageContextInternal(parts, packageContext); if (repoMappingRecorder != null && parts.repo() != null && !parts.repoIsCanonical()) { - repoMappingRecorder.entries.put( + repoMappingRecorder.record( packageContext.currentRepo(), parts.repo(), parsed.getRepository()); } return parsed; @@ -252,12 +252,31 @@ private static Label parseWithPackageContextInternal(Parts parts, PackageContext } /** Records repo mapping entries used by {@link #parseWithPackageContext}. */ - public static final class RepoMappingRecorder { - /** {@code } */ + public interface RepoMappingRecorder { + void record(RepositoryName fromRepo, String apparentRepoName, RepositoryName canonicalRepoName); + + default void record(Table entries) { + for (Table.Cell cell : entries.cellSet()) { + record(cell.getRowKey(), cell.getColumnKey(), cell.getValue()); + } + } + + default void storeInThread(StarlarkThread thread) { + thread.setThreadLocal(RepoMappingRecorder.class, this); + } + } + + /** + * A {@link RepoMappingRecorder} backed by a {@link Table} that is used for BUILD and .bzl load + * threads. + */ + public static final class SimpleRepoMappingRecorder implements RepoMappingRecorder { Table entries = HashBasedTable.create(); - public void mergeEntries(Table entries) { - this.entries.putAll(entries); + @Override + public void record( + RepositoryName fromRepo, String apparentRepoName, RepositoryName canonicalRepoName) { + entries.put(fromRepo, apparentRepoName, canonicalRepoName); } public ImmutableTable recordedEntries() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index bc6c39f4855a30..055b4e329ac32a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -15,14 +15,15 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; -import static java.util.Comparator.naturalOrder; +import static java.util.Map.Entry.comparingByKey; import static java.util.Objects.requireNonNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -44,7 +45,6 @@ import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; -import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -67,7 +67,7 @@ * input is stored as a string, with a prefix denoting its type, followed by a colon, and then the * information identifying that specific input. */ -public abstract sealed class RepoRecordedInput implements Comparable { +public abstract sealed class RepoRecordedInput { /** Represents a parser for a specific type of recorded inputs. */ public abstract static class Parser { /** @@ -84,14 +84,6 @@ public abstract static class Parser { public abstract RepoRecordedInput parse(String s); } - private static final Comparator COMPARATOR = - (o1, o2) -> - o1 == o2 - ? 0 - : Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) - .thenComparing(RepoRecordedInput::toStringInternal) - .compare(o1, o2); - /** * Parses a recorded input from its string representation. * @@ -115,27 +107,80 @@ public static RepoRecordedInput parse(String s) { return NeverUpToDateRepoRecordedInput.PARSE_FAILURE; } + /** A recorded input along with its recorded value. */ + public record WithValue(RepoRecordedInput input, @Nullable String value) { + /** Parses a {@link WithValue} from its string representation. */ + public static Optional parse(String s) { + int sChar = s.indexOf(' '); + if (sChar > 0) { + var input = RepoRecordedInput.parse(unescape(s.substring(0, sChar))); + if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { + return Optional.of(new WithValue(input, unescape(s.substring(sChar + 1)))); + } + } + return Optional.empty(); + } + + /** Converts this {@link WithValue} to a string in a format compatible with {@link #parse}. */ + @Override + public String toString() { + return escape(input.toString()) + " " + escape(value); + } + + @VisibleForTesting + static String escape(String str) { + return str == null + ? "\\0" + : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); + } + + @VisibleForTesting + @Nullable + static String unescape(String str) { + if (str.equals("\\0")) { + return null; // \0 == null string + } + StringBuilder result = new StringBuilder(); + boolean escaped = false; + for (int i = 0; i < str.length(); i++) { + char c = str.charAt(i); + if (escaped) { + if (c == 'n') { // n means new line + result.append("\n"); + } else if (c == 's') { // s means space + result.append(" "); + } else { // Any other escaped characters are just un-escaped + result.append(c); + } + escaped = false; + } else if (c == '\\') { + escaped = true; + } else { + result.append(c); + } + } + return result.toString(); + } + } + /** * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are * missing, the return value should be ignored; callers are responsible for checking {@code * env.valuesMissing()} and triggering a Skyframe restart if needed. */ public static Optional isAnyValueOutdated( - Environment env, - BlazeDirectories directories, - Map recordedInputValues) + Environment env, BlazeDirectories directories, List recordedInputValues) throws InterruptedException { env.getValuesAndExceptions( - recordedInputValues.keySet().stream() - .map(rri -> rri.getSkyKey(directories)) + recordedInputValues.stream() + .map(riv -> riv.input().getSkyKey(directories)) .collect(toImmutableSet())); if (env.valuesMissing()) { return UNDECIDED; } - for (Map.Entry recordedInputValue : - recordedInputValues.entrySet()) { + for (var recordedInput : recordedInputValues) { Optional reason = - recordedInputValue.getKey().isOutdated(env, directories, recordedInputValue.getValue()); + recordedInput.input().isOutdated(env, directories, recordedInput.value()); if (reason.isPresent()) { return reason; } @@ -154,11 +199,6 @@ public final String toString() { return getParser().getPrefix() + ":" + toStringInternal(); } - @Override - public int compareTo(RepoRecordedInput o) { - return COMPARATOR.compare(this, o); - } - /** * Returns the post-colon substring that identifies the specific input: for example, the {@code * MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. @@ -546,12 +586,11 @@ public RepoRecordedInput parse(String s) { final String name; - public static ImmutableSortedMap> wrap( + public static ImmutableMap> wrap( Map> envVars) { return envVars.entrySet().stream() - .collect( - toImmutableSortedMap( - naturalOrder(), e -> new EnvVar(e.getKey()), Map.Entry::getValue)); + .sorted(comparingByKey()) + .collect(toImmutableMap(e -> new EnvVar(e.getKey()), Map.Entry::getValue)); } private EnvVar(String name) { @@ -600,7 +639,7 @@ public Optional isOutdated( // Note that `oldValue` can be null if the env var was not set. if (!Objects.equals(oldValue, v)) { return Optional.of( - "value of %s changed: %s -> %s" + "environment variable %s changed: %s -> %s" .formatted( name, oldValue == null ? "" : "'%s'".formatted(oldValue), @@ -639,6 +678,10 @@ public RecordedRepoMapping(RepositoryName sourceRepo, String apparentName) { this.apparentName = apparentName; } + public String apparentName() { + return apparentName; + } + @Override public boolean equals(Object o) { if (this == o) { 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 0f7e84d82ff90d..b6bb810f0d1fb3 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 @@ -18,11 +18,11 @@ import static com.google.devtools.build.lib.skyframe.RepositoryMappingFunction.REPOSITORY_OVERRIDES; import static java.nio.charset.StandardCharsets.ISO_8859_1; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; 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.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; @@ -64,7 +64,7 @@ import java.io.IOException; import java.util.Map; import java.util.Optional; -import java.util.TreeMap; +import java.util.LinkedHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -671,41 +671,6 @@ public static RepositoryDirectoryValue symlinkRepoRoot( source, /* isFetchingDelayed= */ false, /* excludeFromVendoring= */ true); } - // Escape a value for the marker file - @VisibleForTesting - static String escape(String str) { - return str == null ? "\\0" : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); - } - - // Unescape a value from the marker file - @Nullable - @VisibleForTesting - static String unescape(String str) { - if (str.equals("\\0")) { - return null; // \0 == null string - } - StringBuilder result = new StringBuilder(); - boolean escaped = false; - for (int i = 0; i < str.length(); i++) { - char c = str.charAt(i); - if (escaped) { - if (c == 'n') { // n means new line - result.append("\n"); - } else if (c == 's') { // s means space - result.append(" "); - } else { // Any other escaped characters are just un-escaped - result.append(c); - } - escaped = false; - } else if (c == '\\') { - escaped = true; - } else { - result.append(c); - } - } - return result.toString(); - } - private static class DigestWriter { // Input value map to force repo invalidation upon an invalid marker file. private static final ImmutableMap PARSE_FAILURE = @@ -746,10 +711,10 @@ void writeMarkerFile(Map recordedInputValue StringBuilder builder = new StringBuilder(); builder.append(predeclaredInputHash).append("\n"); for (Map.Entry recordedInput : - new TreeMap(recordedInputValues).entrySet()) { - String key = recordedInput.getKey().toString(); - String value = recordedInput.getValue(); - builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); + new LinkedHashMap(recordedInputValues).entrySet()) { + builder + .append(new RepoRecordedInput.WithValue(recordedInput.getKey(), recordedInput.getValue())) + .append("\n"); } String content = builder.toString(); try { @@ -827,15 +792,13 @@ private static Map readMarkerFile( ""); } firstLineVerified = true; - recordedInputValues = new TreeMap<>(); + recordedInputValues = new LinkedHashMap<>(); } else { - int sChar = line.indexOf(' '); - if (sChar > 0) { - RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar))); - if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { - recordedInputValues.put(input, unescape(line.substring(sChar + 1))); - continue; - } + Optional withValue = + RepoRecordedInput.WithValue.parse(line); + if (withValue.isPresent()) { + recordedInputValues.put(withValue.get().input(), withValue.get().value()); + continue; } // On parse failure, just forget everything else and mark the whole input out of date. return PARSE_FAILURE; @@ -868,6 +831,20 @@ static String computePredeclaredInputHash( .addInt(environ.size()); environ.forEach( (key, value) -> fp.addString(key.toString()).addNullableString(value.orElse(null))); + // The repo mapping entries recorded while loading the repo rule definition are part of the + // predeclared inputs: they influence how labels in the rule's implementation resolve, so a + // change in them must invalidate the repo (and its repo contents cache entries). + var repoMappingEntries = + rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries(); + var repoMappingCells = + repoMappingEntries == null ? ImmutableSet.>of() : repoMappingEntries.cellSet(); + fp.addInt(repoMappingCells.size()); + repoMappingCells.forEach( + entry -> { + fp.addString(entry.getRowKey().getName()); + fp.addString(entry.getColumnKey()); + fp.addString(entry.getValue().getName()); + }); return fp.hexDigestAndReset(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 13b997951b7444..4471325f4649ac 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -239,7 +239,12 @@ public Optional isAnyRecordedInputOutdated( Map recordedInputValues, Environment env) throws InterruptedException { - return RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputValues); + return RepoRecordedInput.isAnyValueOutdated( + env, + directories, + recordedInputValues.entrySet().stream() + .map(e -> new RepoRecordedInput.WithValue(e.getKey(), e.getValue())) + .collect(com.google.common.collect.ImmutableList.toImmutableList())); } public static RootedPath getRootedPathFromLabel(Label label, Environment env) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 0570badd89e5e6..6d7ad863a6c183 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -806,7 +806,7 @@ private BzlLoadValue computeInternalWithCompiledBzl( if (mainRepoMapping == null) { return null; } - Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); + var repoMappingRecorder = new Label.SimpleRepoMappingRecorder(); ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList