From c37e8a32f9465dc443bd6d16a9f13f1ff40209fe Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 28 Jan 2025 08:54:12 -0800 Subject: [PATCH] Record repo mapping entries for labels in module extension tags The mapping of an apparent name in a module extension tag's `attr.label` attribute to the corresponding canonical name has to be recorded in the lockfile so that instantiated repo rules don't reference the stale repos. Fixes #25063 Closes #25067. PiperOrigin-RevId: 720592796 Change-Id: Ia202ca4a8482a81da8085ee18ecaca5fe233bddb --- .../lib/bazel/bzlmod/BazelLockFileValue.java | 2 +- .../bzlmod/RegularRunnableExtension.java | 8 +- .../lib/bazel/bzlmod/StarlarkBazelModule.java | 7 +- .../build/lib/packages/LabelConverter.java | 24 ++++- .../bzlmod/ModuleExtensionResolutionTest.java | 18 ++++ .../bazel/bzlmod/StarlarkBazelModuleTest.java | 11 ++- .../py/bazel/bzlmod/bazel_lockfile_test.py | 89 +++++++++++++++++++ src/test/tools/bzlmod/MODULE.bazel.lock | 2 +- 8 files changed, 150 insertions(+), 11 deletions(-) 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 06549401f7137b..3c0d144f8797de 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 @@ -36,7 +36,7 @@ public abstract class BazelLockFileValue implements SkyValue { // NOTE: See "HACK" note in BazelLockFileModule. While this hack exists, normal increments of the // lockfile version need to be done by 2 at a time (i.e. keep LOCK_FILE_VERSION an odd number). - public static final int LOCK_FILE_VERSION = 11; + public static final int LOCK_FILE_VERSION = 13; @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE; 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 be4672af5d7cfe..3ed236655baef3 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 @@ -272,7 +272,7 @@ private RunModuleExtensionResult runInternal( try (Mutability mu = Mutability.create("module extension", usagesValue.getExtensionUniqueName()); ModuleExtensionContext moduleContext = - createContext(env, usagesValue, starlarkSemantics, extensionId)) { + createContext(env, usagesValue, starlarkSemantics, extensionId, repoMappingRecorder)) { StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics); thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); threadContext.storeInThread(thread); @@ -339,7 +339,8 @@ private ModuleExtensionContext createContext( Environment env, SingleExtensionUsagesValue usagesValue, StarlarkSemantics starlarkSemantics, - ModuleExtensionId extensionId) + ModuleExtensionId extensionId, + Label.RepoMappingRecorder repoMappingRecorder) throws ExternalDepsException { Path workingDirectory = directories @@ -354,7 +355,8 @@ private ModuleExtensionContext createContext( abridgedModule, extension, usagesValue.getRepoMappings().get(moduleKey), - usagesValue.getExtensionUsages().get(moduleKey))); + usagesValue.getExtensionUsages().get(moduleKey), + repoMappingRecorder)); } ModuleExtensionUsage rootUsage = usagesValue.getExtensionUsages().get(ModuleKey.ROOT); boolean rootModuleHasNonDevDependency = diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java index 30969644e54d1b..a33644fedf27cb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.packages.LabelConverter; @@ -107,12 +108,14 @@ public static StarlarkBazelModule create( AbridgedModule module, ModuleExtension extension, RepositoryMapping repoMapping, - @Nullable ModuleExtensionUsage usage) + @Nullable ModuleExtensionUsage usage, + Label.RepoMappingRecorder repoMappingRecorder) throws ExternalDepsException { LabelConverter labelConverter = new LabelConverter( PackageIdentifier.create(repoMapping.ownerRepo(), PathFragment.EMPTY_FRAGMENT), - repoMapping); + repoMapping, + repoMappingRecorder); ImmutableList tags = usage == null ? ImmutableList.of() : usage.getTags(); HashMap> typeCheckedTags = new HashMap<>(); for (String tagClassName : extension.getTagClasses().keySet()) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/LabelConverter.java b/src/main/java/com/google/devtools/build/lib/packages/LabelConverter.java index 64ac5ba4084d5a..4d457f39c7f5fc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/LabelConverter.java +++ b/src/main/java/com/google/devtools/build/lib/packages/LabelConverter.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.cmdline.RepositoryMapping; import java.util.HashMap; import java.util.Map; +import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkThread; /** @@ -42,9 +43,17 @@ public static LabelConverter forBzlEvaluatingThread(StarlarkThread thread) { private final Label.PackageContext packageContext; private final Map labelCache = new HashMap<>(); + @Nullable private final Label.RepoMappingRecorder repoMappingRecorder; - public LabelConverter(Label.PackageContext packageContext) { + private LabelConverter( + Label.PackageContext packageContext, + @Nullable Label.RepoMappingRecorder repoMappingRecorder) { this.packageContext = packageContext; + this.repoMappingRecorder = repoMappingRecorder; + } + + public LabelConverter(Label.PackageContext packageContext) { + this(packageContext, null); } /** Creates a label converter using the given base package and repo mapping. */ @@ -52,6 +61,17 @@ public LabelConverter(PackageIdentifier base, RepositoryMapping repositoryMappin this(Label.PackageContext.of(base, repositoryMapping)); } + /** + * Creates a label converter using the given base package and repo mapping, recording all repo + * mapping lookups in the given recorder. + */ + public LabelConverter( + PackageIdentifier base, + RepositoryMapping repositoryMapping, + Label.RepoMappingRecorder repoMappingRecorder) { + this(Label.PackageContext.of(base, repositoryMapping), repoMappingRecorder); + } + /** Returns the base package identifier that relative labels will be resolved against. */ PackageIdentifier getBasePackage() { return packageContext.packageIdentifier(); @@ -65,7 +85,7 @@ public Label convert(String input) throws LabelSyntaxException { // label-strings across all their attribute values. Label converted = labelCache.get(input); if (converted == null) { - converted = Label.parseWithPackageContext(input, packageContext); + converted = Label.parseWithPackageContext(input, packageContext, repoMappingRecorder); labelCache.put(input, converted); } return converted; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 45d76832687d8c..81663cbf665af6 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -894,6 +894,24 @@ public void labels_passedOnToRepoRule() throws Exception { } assertThat(result.get(skyKey).getModule().getGlobal("data")) .isEqualTo("get up at 6am. go to bed at 11pm."); + + SkyKey extensionSkyKey = + SingleExtensionValue.key( + ModuleExtensionId.create( + Label.parseCanonicalUnchecked("@@ext+//:defs.bzl"), "ext", Optional.empty())); + EvaluationResult extensionResult = + evaluator.evaluate(ImmutableList.of(extensionSkyKey), evaluationContext); + if (extensionResult.hasError()) { + throw extensionResult.getError().getException(); + } + assertThat( + extensionResult + .get(extensionSkyKey) + .getLockFileInfo() + .get() + .moduleExtension() + .getRecordedRepoMappingEntries()) + .containsCell(RepositoryName.create("foo+"), "bar", RepositoryName.create("bar+")); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java index b300c58e81172b..8b39f847a5dcdd 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.util.FileTypeSet; @@ -89,6 +90,7 @@ public void basic() throws Exception { Module module = buildModule("foo", "1.0").setKey(fooKey).addDep("bar", barKey).build(); AbridgedModule abridgedModule = AbridgedModule.from(module); + Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); StarlarkBazelModule moduleProxy = StarlarkBazelModule.create( abridgedModule, @@ -97,7 +99,8 @@ public void basic() throws Exception { ImmutableMap.of( fooKey, fooKey.getCanonicalRepoNameWithoutVersionForTesting(), barKey, barKey.getCanonicalRepoNameWithoutVersionForTesting())), - usage); + usage, + repoMappingRecorder); assertThat(moduleProxy.getName()).isEqualTo("foo"); assertThat(moduleProxy.getVersion()).isEqualTo("1.0"); @@ -124,6 +127,9 @@ public void basic() throws Exception { StarlarkList.immutableOf( Label.parseCanonical("@@foo~//:pom.xml"), Label.parseCanonical("@@bar~//:pom.xml"))); + + assertThat(repoMappingRecorder.recordedEntries()) + .containsCell(RepositoryName.create("foo+"), "bar", RepositoryName.create("bar+")); } @Test @@ -145,7 +151,8 @@ public void unknownTagClass() throws Exception { module.getRepoMappingWithBazelDepsOnly( ImmutableMap.of( fooKey, fooKey.getCanonicalRepoNameWithoutVersionForTesting())), - usage)); + usage, + new Label.RepoMappingRecorder())); assertThat(e).hasMessageThat().contains("does not have a tag class named blep"); } } diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index cb2f4cf2e127f8..7d6096dafb7f45 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -1883,6 +1883,95 @@ def testExtensionRepoMappingChange_BzlInit(self): self.assertNotIn('ran the extension!', stderr) self.assertIn('STR=@@bar~//:lib_foo', stderr) + def testExtensionRepoMappingChange_tag(self): + # Regression test for #20721 + self.main_registry.createCcModule('foo', '1.0') + self.main_registry.createCcModule('bar', '1.0') + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name="foo",version="1.0",repo_name="repo_name")', + 'bazel_dep(name="bar",version="1.0")', + 'ext = use_extension(":ext.bzl", "ext")', + 'ext.tag(label = "@repo_name//:lib_foo")', + '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 = mctx.modules[0].tags.tag[0].label)', + 'tag = tag_class(', + ' attrs = {', + ' "label": attr.label(),', + ' },', + ')', + 'ext = module_extension(_ext_impl, tag_classes={"tag": tag})', + ], + ) + + _, _, stderr = self.RunBazel(['build', ':lol']) + self.assertIn('STR=@@foo+//:lib_foo', '\n'.join(stderr)) + + # Shutdown bazel to make sure we rely on the lockfile and not skyframe + self.RunBazel(['shutdown']) + _, _, stderr = self.RunBazel(['build', ':lol']) + self.assertNotIn('ran the extension!', '\n'.join(stderr)) + + # Shutdown bazel to make sure we rely on the lockfile and not skyframe + self.RunBazel(['shutdown']) + # Now, for something spicy: let repo_name point to bar and change nothing + # else. The extension should rerun despite the lockfile being present, and + # no usages or .bzl files having changed. + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name="foo",version="1.0")', + 'bazel_dep(name="bar",version="1.0",repo_name="repo_name")', + 'ext = use_extension(":ext.bzl", "ext")', + 'ext.tag(label = "@repo_name//:lib_foo")', + 'use_repo(ext, "repo")', + ], + ) + _, _, stderr = self.RunBazel(['build', ':lol']) + stderr = '\n'.join(stderr) + self.assertIn('ran the extension!', stderr) + self.assertIn('STR=@@bar+//:lib_foo', stderr) + + # Shutdown bazel to make sure we rely on the lockfile and not skyframe + self.RunBazel(['shutdown']) + # More spicy! change the repo_name of foo, but nothing else. + # The extension should NOT rerun, since it never used the @other_name repo + # mapping. + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name="foo",version="1.0",repo_name="other_name")', + 'bazel_dep(name="bar",version="1.0",repo_name="repo_name")', + 'ext = use_extension(":ext.bzl", "ext")', + 'ext.tag(label = "@repo_name//:lib_foo")', + 'use_repo(ext, "repo")', + ], + ) + _, _, stderr = self.RunBazel(['build', ':lol']) + stderr = '\n'.join(stderr) + self.assertNotIn('ran the extension!', stderr) + self.assertIn('STR=@@bar+//:lib_foo', stderr) + def testExtensionRepoMappingChange_loadsAndRepoRelativeLabels(self): # Regression test for #20721; same test as above, except that the call to # Label() in ext.bzl is now moved to @{foo,bar}//:defs.bzl and doesn't diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index 07e9c2b5e90a80..09cda8f21803dd 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -1,5 +1,5 @@ { - "lockFileVersion": 11, + "lockFileVersion": 13, "registryFileHashes": { "https://bcr.bazel.build/bazel_registry.json": "8a28e4aff06ee60aed2a8c281907fb8bcbf3b753c91fb5a5c57da3215d5b3497", "https://bcr.bazel.build/modules/abseil-cpp/20210324.2/MODULE.bazel": "7cd0312e064fde87c8d1cd79ba06c876bd23630c83466e9500321be55c96ace2",