From 6272705c5e347b0738a0783438c63eabafc9f5c5 Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Wed, 10 Jan 2024 14:08:07 -0800 Subject: [PATCH] Let .bzl files record their usages of repo mapping 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 +- .../skyframe/RepositoryMappingFunction.java | 2 +- .../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 | 260 ++++++++++++++++++ 11 files changed, 359 insertions(+), 30 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 01148cc900cd95..0dc11096e070ed 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; } @@ -817,20 +819,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; } @@ -849,7 +850,7 @@ public ImmutableMap getEnvVars() { @Override public byte[] getBzlTransitiveDigest() { - return bazelModuleContext.bzlTransitiveDigest(); + return BazelModuleContext.of(bzlLoadValue.getModule()).bzlTransitiveDigest(); } @Nullable @@ -864,12 +865,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 e7578d30432695..494b991f53cbfb 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 @@ -229,6 +229,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 27e14bc6098de7..d2dfb7648e4a81 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2266,6 +2266,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 08a3ee4e2d80f7..9c57bac68f819c 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 @@ -762,6 +762,7 @@ private BzlLoadValue computeInternalWithCompiledBzl( if (repoMapping == null) { return null; } + Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList