From 476e183da5a1d41bb83f2bace5f078864c406f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Wed, 10 Jan 2024 21:44:10 -0500 Subject: [PATCH] [7.0.1] Let .bzl files record their usages of repo mapping (#20848) In the same vein as https://github.com/bazelbuild/bazel/pull/20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name. See https://github.com/bazelbuild/bazel/issues/20721#issuecomment-1883849462 for a more detailed explanation for this change, and the test cases in this commit for more potential triggers. Fixes https://github.com/bazelbuild/bazel/issues/20721 Closes #20830. PiperOrigin-RevId: 597351525 Change-Id: I8f6ed297b81d55f7476a93bdc6668e1e1dcbe536 --- .../bzlmod/SingleExtensionEvalFunction.java | 30 +- .../devtools/build/lib/cmdline/Label.java | 4 + .../google/devtools/build/lib/skyframe/BUILD | 1 + .../build/lib/skyframe/BzlLoadFunction.java | 32 ++- .../build/lib/skyframe/BzlLoadValue.java | 17 +- .../lib/skyframe/RepositoryMappingValue.java | 6 + .../lib/skyframe/BzlLoadFunctionTest.java | 25 +- .../build/lib/skyframe/serialization/BUILD | 1 + .../serialization/BzlLoadValueCodecTest.java | 11 +- .../py/bazel/bzlmod/bazel_lockfile_test.py | 265 ++++++++++++++++++ src/test/py/bazel/bzlmod/test_utils.py | 16 +- 11 files changed, 372 insertions(+), 36 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 8838dff04fd571..753a8cc44a2dde 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 @@ -348,21 +348,23 @@ private static boolean didRepoMappingsChange( .map(RepositoryMappingValue::key) .collect(toImmutableSet())); if (env.valuesMissing()) { - // This shouldn't really happen, since the RepositoryMappingValues of any recorded repos - // should have already been requested by the time we load the .bzl for the extension. And this - // method is only called if the transitive .bzl digest hasn't changed. - // However, we pretend it could happen anyway because we're good citizens. + // 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(RepositoryMappingValue.key(cell.getRowKey())); if (repoMappingValue == null) { - // Again, this shouldn't happen. But anyway. throw new NeedsSkyframeRestartException(); } - if (!cell.getValue() - .equals(repoMappingValue.getRepositoryMapping().get(cell.getColumnKey()))) { + // 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.getRepositoryMapping().get(cell.getColumnKey()))) { // Wee woo wee woo -- diff detected! return true; } @@ -806,20 +808,19 @@ private RegularRunnableExtension loadRegularRunnableExtension( if (envVars == null) { return null; } - return new RegularRunnableExtension( - BazelModuleContext.of(bzlLoadValue.getModule()), extension, envVars); + return new RegularRunnableExtension(bzlLoadValue, extension, envVars); } private final class RegularRunnableExtension implements RunnableExtension { - private final BazelModuleContext bazelModuleContext; + private final BzlLoadValue bzlLoadValue; private final ModuleExtension extension; private final ImmutableMap envVars; RegularRunnableExtension( - BazelModuleContext bazelModuleContext, + BzlLoadValue bzlLoadValue, ModuleExtension extension, ImmutableMap envVars) { - this.bazelModuleContext = bazelModuleContext; + this.bzlLoadValue = bzlLoadValue; this.extension = extension; this.envVars = envVars; } @@ -838,7 +839,7 @@ public ImmutableMap getEnvVars() { @Override public byte[] getBzlTransitiveDigest() { - return bazelModuleContext.bzlTransitiveDigest(); + return BazelModuleContext.of(bzlLoadValue.getModule()).bzlTransitiveDigest(); } @Nullable @@ -853,12 +854,13 @@ public RunModuleExtensionResult run( new ModuleExtensionEvalStarlarkThreadContext( usagesValue.getExtensionUniqueName() + "~", extensionId.getBzlFileLabel().getPackageIdentifier(), - bazelModuleContext.repoMapping(), + BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(), directories, env.getListener()); ModuleExtensionContext moduleContext; Optional moduleExtensionMetadata; var repoMappingRecorder = new Label.RepoMappingRecorder(); + repoMappingRecorder.mergeEntries(bzlLoadValue.getRecordedRepoMappings()); try (Mutability mu = Mutability.create("module extension", usagesValue.getExtensionUniqueName())) { StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics); 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 d0a033d4b23bdc..242b7404cf3732 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 @@ -231,6 +231,10 @@ public static final class RepoMappingRecorder { /** {@code } */ Table entries = HashBasedTable.create(); + public void mergeEntries(Table entries) { + this.entries.putAll(entries); + } + public ImmutableTable recordedEntries() { return ImmutableTable.builder() .orderRowsBy(Comparator.comparing(RepositoryName::getName)) 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 bb7f7b2472469e..a7732fa14faf9c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2190,6 +2190,7 @@ java_library( "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:auto_value", "//third_party:guava", + "//third_party:jsr305", ], ) 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 1d4cc4ba8979b6..048d6c252f966a 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 @@ -765,6 +765,7 @@ private BzlLoadValue computeInternalWithCompiledBzl( if (repoMapping == null) { return null; } + Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList