From 92f4ab00c0146f1bc4e68beb61e4461c9b0cd4dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Mon, 22 Jan 2024 16:51:41 -0500 Subject: [PATCH 1/2] Disregard WORKSPACE while verifying lockfile repo mapping entries in extension eval See code comment and linked issue for more information. Fixes #20942. --- .../bzlmod/SingleExtensionEvalFunction.java | 13 +++++- .../py/bazel/bzlmod/bazel_lockfile_test.py | 44 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) 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 0dc11096e070ed..292c52ed54823e 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 @@ -342,10 +342,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, + // so we only request the main repo mapping _without_ WORKSPACE repos. See #20942 for more + // information. SkyframeLookupResult result = env.getValuesAndExceptions( recordedRepoMappings.rowKeySet().stream() - .map(RepositoryMappingValue::key) + .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 @@ -354,7 +361,9 @@ private static boolean didRepoMappingsChange( } for (Table.Cell cell : recordedRepoMappings.cellSet()) { RepositoryMappingValue repoMappingValue = - (RepositoryMappingValue) result.get(RepositoryMappingValue.key(cell.getRowKey())); + (RepositoryMappingValue) result.get(cell.getRowKey().isMain() + ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS + : RepositoryMappingValue.key(cell.getRowKey())); if (repoMappingValue == null) { throw new NeedsSkyframeRestartException(); } diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index d48ece85d1e2ce..8a71dd21f2ab31 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -1833,6 +1833,50 @@ def testExtensionRepoMappingChange_sourceRepoNoLongerExistent(self): self.assertIn('ran the extension!', '\n'.join(stderr)) self.assertIn('STR=@@quux~1.0//:quux.h', '\n'.join(stderr)) + def testExtensionRepoMappingChange_mainRepoEvalCycleWithWorkspace(self): + # Regression test for #20942 + self.main_registry.createCcModule('foo', '1.0') + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name="foo",version="1.0")', + 'ext = use_extension(":ext.bzl", "ext")', + 'use_repo(ext, "repo")', + ], + ) + self.ScratchFile( + 'BUILD.bazel', + [ + 'load("@repo//:defs.bzl", "STR")', + 'print("STR="+STR)', + 'filegroup(name="lol")', + ], + ) + self.ScratchFile( + 'ext.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("BUILD")', + ' rctx.file("defs.bzl", "STR = " + repr(str(rctx.attr.value)))', + 'repo = repository_rule(_repo_impl,attrs={"value":attr.label()})', + 'def _ext_impl(mctx):', + ' print("ran the extension!")', + ' repo(name = "repo", value = Label("@foo//:lib_foo"))', + 'ext = module_extension(_ext_impl)', + ], + ) + # any `load` in WORKSPACE should trigger the bug + self.ScratchFile('WORKSPACE.bzlmod', ['load("@repo//:defs.bzl","STR")']) + + _, _, stderr = self.RunBazel(['build', '--enable_workspace', ':lol']) + self.assertIn('STR=@@foo~1.0//:lib_foo', '\n'.join(stderr)) + + # Shutdown bazel to make sure we rely on the lockfile and not skyframe + self.RunBazel(['shutdown']) + # Build again. This should _NOT_ trigger a failure! + _, _, stderr = self.RunBazel(['build', '--enable_workspace', ':lol']) + self.assertNotIn('ran the extension!', '\n'.join(stderr)) + if __name__ == '__main__': absltest.main() From a5f4f887b04ee73f0e0f47465514c31d313de5e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Mon, 22 Jan 2024 17:32:50 -0500 Subject: [PATCH 2/2] add a small comment --- .../build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 292c52ed54823e..5901f370701fca 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 @@ -344,9 +344,9 @@ private static boolean didRepoMappingsChange( 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, - // so we only request the main repo mapping _without_ WORKSPACE repos. See #20942 for more - // information. + // 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()