From b66010cd4dd14f282bbc554ddb19df42f35dc01c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 23 Jun 2026 18:14:44 +0200 Subject: [PATCH] Clarify the invalidation of REPO_CONTENTS_CACHE_DIRS FileStateValues Since this behavior is quite surprising (it definitely was to the author), this change also improves the test coverage for repo contents cache deletion by asserting that non-BUILD files within it actually exist on disk rather than just exist from the point of Skyframe. Also fix a crash observed while working on the test improvements. Closes #28222. PiperOrigin-RevId: 855225639 Change-Id: Ie4a88e93d14a4f4b7bb5217fc924e998a1779ccd (cherry picked from commit 4839f46d9de21d2e5f59d1cf0975c2adc342a796) --- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/DirtinessCheckerUtils.java | 4 +- .../lib/skyframe/ExternalFilesHelper.java | 16 +++--- .../lib/skyframe/SkyframeErrorProcessor.java | 11 +++- .../build/lib/skyframe/SkyframeExecutor.java | 2 + src/test/py/bazel/bzlmod/bazel_module_test.py | 30 +++++++++++ .../bazel/bzlmod/repo_contents_cache_test.py | 54 ++++++++++++------- 7 files changed, 91 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index c2210207ecb5fa..4100aa78ef4df4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2566,6 +2566,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:constraints/top_level_constraint_semantics", "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/causes", 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 fd68caaf19fb8b..c6f310b42a7547 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java @@ -165,8 +165,10 @@ private static boolean isCacheableType(FileType fileType) { return true; case EXTERNAL_REPO: case OUTPUT: - case REPO_CONTENTS_CACHE_DIRS: return false; + case REPO_CONTENTS_CACHE_DIRS: + throw new IllegalStateException( + "Repo contents cache dirs are not expected to be checked for dirtiness"); } throw new AssertionError("Unknown type " + fileType); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index e3d59041344bda..b75e78cc79a8f7 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 @@ -180,14 +180,16 @@ public enum FileType { /** * 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. + * but may be deleted when users delete the entire repo contents cache. However, they are always + * recreated by Bazel before they are used and/or depended on via Skyframe. They are thus + * immutably present from the perspective of Skyframe and don't require invalidation. * - *

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

The contents of these directories are considered EXTERNAL as they carry UUID names - * and are thus never reused. + *

Note: If these directories ever need to be checked for dirtiness during diffing, they have + * to be made non-cacheable according to {@link + * DirtinessCheckerUtils.ExternalDirtinessChecker#isCacheableType} so that they are not locked + * in as non-existent if they have been removed. This would result in FileValues for files below + * them (the actual repo contents, of type EXTERNAL) being locked in as non-existent too, + * even after a refetch of the repo has added a new cache entry. */ REPO_CONTENTS_CACHE_DIRS, } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java index 84284a27743514..9c8502a561fd89 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.actions.TestExecException; import com.google.devtools.build.lib.actions.TopLevelOutputException; import com.google.devtools.build.lib.analysis.AnalysisFailureEvent; +import com.google.devtools.build.lib.bazel.bzlmod.ExternalDepsException; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics.TargetCompatibilityCheckException; import com.google.devtools.build.lib.bugreport.BugReport; @@ -540,6 +541,13 @@ && isExecutionCycle(errorInfo.getCycleInfo())) { configurationIdMessage(ctKey.getConfigurationKey()), ((NoSuchThingException) exception).getDetailedExitCode()); analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause); + } else if (exception instanceof ExternalDepsException externalDepsException) { + AnalysisFailedCause analysisFailedCause = + new AnalysisFailedCause( + topLevelLabel, + configurationIdMessage(ctKey.getConfigurationKey()), + externalDepsException.getDetailedExitCode()); + analysisRootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, analysisFailedCause); } else if (exception instanceof TargetCompatibilityCheckException) { analysisRootCauses = NestedSetBuilder.emptySet(Order.STABLE_ORDER); } else if (isExecutionException(exception)) { @@ -779,7 +787,8 @@ private static DetailedException convertToAnalysisException(Throwable cause) { // analyze with --nokeep_going. if (cause instanceof SaneAnalysisException || cause instanceof NoSuchTargetException - || cause instanceof NoSuchPackageException) { + || cause instanceof NoSuchPackageException + || cause instanceof ExternalDepsException) { return (DetailedException) cause; } return null; 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 a0118e6eb5897c..79c92ea8506b92 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 @@ -3470,6 +3470,8 @@ protected void handleDiffsWithMissingDiffInformation( fileTypesToCheck.add(FileType.OUTPUT); } if (!fileTypesToCheck.isEmpty()) { + // FileType.REPO_CONTENTS_CACHE_DIRS is intentionally never checked here. See the comment on + // that enum constant for details. dirtinessCheckers = Iterables.concat( dirtinessCheckers, diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index ed092907c07e8a..5d1e11037cc48d 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -1540,6 +1540,36 @@ def testUnknownRegistryInModuleMirrors(self): ) self.assertIn('https://unknown-registry.example.com', stderr) + def testInvalidRepoRuleReferencedByTargetDoesNotCrash(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'repo = use_repo_rule("//:repo.bzl", "repo")', + 'repo(name = "my_repo")', + ], + ) + self.ScratchFile( + 'BUILD', + [ + 'genrule(', + ' name = "gen",', + ' srcs = ["@my_repo//:a.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $(SRCS) > $(OUTS)",', + ')', + ], + ) + self.ScratchFile('repo.bzl', ['nonsense']) + + exit_code, _, stderr = self.RunBazel( + ['build', '//:gen'], + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + stderr = '\n'.join(stderr) + self.assertNotIn('FATAL', stderr) + self.assertIn("compilation of module 'repo.bzl' failed", stderr) + if __name__ == '__main__': absltest.main() 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 961323f952594a..60ea22b616d088 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -503,77 +503,93 @@ def doTestRepoContentsCacheDeleted(self, check_external_repository_files): 'repo(name = "my_repo")', ], ) - self.ScratchFile('workspace/BUILD.bazel') + self.ScratchFile( + 'workspace/BUILD.bazel', + [ + 'genrule(', + ' name = "gen",', + ' srcs = ["@my_repo//:haha", "in.txt"],', + ' outs = ["out.txt"],', + ' cmd = "cat $(SRCS) > $(OUTS)",', + ')', + ], + ) self.ScratchFile( 'workspace/repo.bzl', [ 'def _repo_impl(rctx):', - ' rctx.file("BUILD", "filegroup(name=\'haha\')")', + ( + ' rctx.file("BUILD", "filegroup(name=\'haha\',' + " srcs=['a.txt'], visibility=['//visibility:public'])\")" + ), + ' rctx.file("a.txt", "hello world")', ' print("JUST FETCHED")', ' return rctx.repo_metadata(reproducible=True)', 'repo = repository_rule(_repo_impl)', ], ) # First fetch: not cached + self.ScratchFile('workspace/in.txt', ['1']) _, _, stderr = self.RunBazel( [ 'build', - '@my_repo//:haha', + '//:gen', ] + extra_args, cwd=workspace, ) self.assertIn('JUST FETCHED', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world1\n') # Second fetch: cached + self.ScratchFile('workspace/in.txt', ['2']) _, _, stderr = self.RunBazel( [ 'build', - '@my_repo//:haha', + '//:gen', ] + extra_args, cwd=workspace, ) self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world2\n') # 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') + self.ScratchFile('workspace/in.txt', ['3']) _, _, stderr = self.RunBazel( - [ - 'build', - '@my_repo//:haha', - ] - + extra_args, + ['build', '//:gen'] + extra_args, cwd=workspace, ) stderr = '\n'.join(stderr) self.assertIn('JUST FETCHED', stderr) self.assertNotIn('WARNING', stderr) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world3\n') # Second fetch after deletion: cached + self.ScratchFile('workspace/in.txt', ['4']) _, _, stderr = self.RunBazel( - [ - 'build', - '@my_repo//:haha', - ] - + extra_args, + ['build', '//:gen'] + extra_args, cwd=workspace, ) self.assertNotIn('JUST FETCHED', '\n'.join(stderr)) self.assertNotIn('WARNING', '\n'.join(stderr)) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world4\n') # 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') + self.ScratchFile('workspace/in.txt', ['5']) _, _, stderr = self.RunBazel( - [ - 'build', - '@my_repo//:haha', - ] + ['build', '//:gen'] + extra_args + [ '--repo_contents_cache=%s' % repo_contents_cache + '2', @@ -583,6 +599,8 @@ def doTestRepoContentsCacheDeleted(self, check_external_repository_files): stderr = '\n'.join(stderr) self.assertIn('JUST FETCHED', stderr) self.assertNotIn('WARNING', stderr) + with open(os.path.join(workspace, 'bazel-bin/out.txt'), 'r') as f: + self.assertEqual(f.read(), 'hello world5\n') def testRepoContentsCacheDeleted_withCheckExternalRepositoryFiles(self): self.doTestRepoContentsCacheDeleted(check_external_repository_files=True)