From af5fe43a926777f44caa1697eb9c64dd9fa3a457 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 25 Apr 2024 15:45:01 -0700 Subject: [PATCH] [7.2.0] Replace most Bzlmod events with a Skyframe graph lookup Retaining `Postable`s across the graph increases the memory usage of every dependent Skyframe node. `ModuleExtensionResolutionEvent` is replaced with a lookup of the "done" Skyframe nodes for extension evaluation in `BazelLockFileModule#afterCommand`. `RootModuleFileFixupEvent` is replaced with direct storage in `SingleExtensionEvalValue`. The validation of repos imported from an extension is moved into a separate `SkyFunction`. This simplifies `bazel mod tidy`, which no longer needs to swallow certain exceptions, and allows cache hits when only the (invalid) imports of an extension are modified even without the lockfile. The `BazelModuleResolutionEvent` is kept for now, but will become obsolete with the new lockfile format. Work towards #20369 Closes #22058. PiperOrigin-RevId: 628213907 Change-Id: I8ba19f5151a8183be5051c8d9280f93476db2272 --- .../lib/bazel/BazelRepositoryModule.java | 2 + .../devtools/build/lib/bazel/bzlmod/BUILD | 16 +- .../bazel/bzlmod/BazelFetchAllFunction.java | 16 +- .../lib/bazel/bzlmod/BazelLockFileModule.java | 98 ++++++----- .../bazel/bzlmod/BazelModTidyFunction.java | 37 ++--- .../lib/bazel/bzlmod/BazelModTidyValue.java | 7 + .../bzlmod/BazelModuleInspectorFunction.java | 16 +- .../bazel/bzlmod/LockFileModuleExtension.java | 7 + .../bazel/bzlmod/ModuleExtensionMetadata.java | 36 ++-- ...leExtensionRepoMappingEntriesFunction.java | 4 +- .../ModuleExtensionResolutionEvent.java | 69 -------- ...xupEvent.java => RootModuleFileFixup.java} | 29 +--- .../bzlmod/SingleExtensionEvalFunction.java | 86 +++++----- .../bazel/bzlmod/SingleExtensionFunction.java | 81 +++++++++ ...alValue.java => SingleExtensionValue.java} | 76 +++++++-- .../devtools/build/lib/bazel/commands/BUILD | 2 +- .../build/lib/bazel/commands/ModCommand.java | 15 +- .../lib/skyframe/BzlLoadCycleReporter.java | 4 +- .../lib/skyframe/BzlmodRepoCycleReporter.java | 4 + .../lib/skyframe/BzlmodRepoRuleFunction.java | 12 +- .../build/lib/skyframe/SkyFunctions.java | 4 +- .../build/lib/analysis/util/AnalysisMock.java | 2 + .../devtools/build/lib/bazel/bzlmod/BUILD | 4 +- .../bzlmod/ModuleExtensionResolutionTest.java | 157 ++++++++++++------ .../py/bazel/bzlmod/bazel_lockfile_test.py | 108 ++++++++++++ 25 files changed, 564 insertions(+), 328 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java rename src/main/java/com/google/devtools/build/lib/bazel/bzlmod/{RootModuleFileFixupEvent.java => RootModuleFileFixup.java} (61%) create mode 100644 src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java rename src/main/java/com/google/devtools/build/lib/bazel/bzlmod/{SingleExtensionEvalValue.java => SingleExtensionValue.java} (55%) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 9fe49abca95274..6dd19bc135998c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.RegistryFunction; import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction; import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionEvalFunction; +import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionFunction; import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionUsagesFunction; import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsFunction; import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil; @@ -264,6 +265,7 @@ SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace .addSkyFunction(SkyFunctions.BAZEL_MOD_TIDY, new BazelModTidyFunction()) .addSkyFunction(SkyFunctions.BAZEL_MODULE_INSPECTION, new BazelModuleInspectorFunction()) .addSkyFunction(SkyFunctions.BAZEL_MODULE_RESOLUTION, new BazelModuleResolutionFunction()) + .addSkyFunction(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) .addSkyFunction(SkyFunctions.SINGLE_EXTENSION_EVAL, singleExtensionEvalFunction) .addSkyFunction(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction()) .addSkyFunction( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index e3613bf68db6f8..78fad249478d4c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -106,6 +106,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/skyframe", "//third_party:flogger", "//third_party:guava", "//third_party:jsr305", @@ -133,7 +134,6 @@ java_library( "ModuleExtensionEvalFactors.java", "ModuleExtensionEvalStarlarkThreadContext.java", "ModuleExtensionRepoMappingEntriesValue.java", - "ModuleExtensionResolutionEvent.java", "ModuleFileGlobals.java", "ModuleFileValue.java", "ModuleOverride.java", @@ -143,8 +143,8 @@ java_library( "RegistryKey.java", "RegistryOverride.java", "RepoSpecKey.java", - "SingleExtensionEvalValue.java", "SingleExtensionUsagesValue.java", + "SingleExtensionValue.java", "SingleVersionOverride.java", "YankedVersionsValue.java", ], @@ -156,6 +156,7 @@ java_library( ":module_extension_metadata", ":registry", ":repo_rule_creator", + ":root_module_file_fixup", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/cmdline", @@ -199,6 +200,7 @@ java_library( "RepoSpecFunction.java", "Selection.java", "SingleExtensionEvalFunction.java", + "SingleExtensionFunction.java", "SingleExtensionUsagesFunction.java", "StarlarkBazelModule.java", "TypeCheckedTag.java", @@ -214,6 +216,7 @@ java_library( ":registry", ":repo_rule_creator", ":resolution", + ":root_module_file_fixup", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", @@ -293,6 +296,7 @@ java_library( ], deps = [ ":resolution", + ":root_module_file_fixup", "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", @@ -314,6 +318,7 @@ java_library( ":exception", ":resolution", ":resolution_impl", + ":root_module_file_fixup", ":tidy", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", @@ -371,7 +376,7 @@ java_library( deps = [ ":common", ":module_extension", - ":module_file_fixup_event", + ":root_module_file_fixup", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", @@ -386,13 +391,12 @@ java_library( ) java_library( - name = "module_file_fixup_event", + name = "root_module_file_fixup", srcs = [ - "RootModuleFileFixupEvent.java", + "RootModuleFileFixup.java", ], deps = [ ":module_extension", - "//src/main/java/com/google/devtools/build/lib/events", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelFetchAllFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelFetchAllFunction.java index 40dfddf9e5ab50..17ff239e0546a7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelFetchAllFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelFetchAllFunction.java @@ -57,16 +57,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept // 2. Run every extension found in the modules & collect its generated repos ImmutableSet extensionIds = depGraphValue.getExtensionUsagesTable().rowKeySet(); - ImmutableSet singleEvalKeys = - extensionIds.stream().map(SingleExtensionEvalValue::key).collect(toImmutableSet()); - SkyframeLookupResult singleEvalValues = env.getValuesAndExceptions(singleEvalKeys); - for (SkyKey singleEvalKey : singleEvalKeys) { - SingleExtensionEvalValue singleEvalValue = - (SingleExtensionEvalValue) singleEvalValues.get(singleEvalKey); - if (singleEvalValue == null) { + ImmutableSet singleExtensionKeys = + extensionIds.stream().map(SingleExtensionValue::key).collect(toImmutableSet()); + SkyframeLookupResult singleExtensionValues = env.getValuesAndExceptions(singleExtensionKeys); + for (SkyKey singleExtensionKey : singleExtensionKeys) { + SingleExtensionValue singleExtensionValue = + (SingleExtensionValue) singleExtensionValues.get(singleExtensionKey); + if (singleExtensionValue == null) { return null; } - reposToFetch.addAll(singleEvalValue.getCanonicalRepoNameToInternalNames().keySet()); + reposToFetch.addAll(singleExtensionValue.getCanonicalRepoNameToInternalNames().keySet()); } // 3. If this is fetch configure, get repo rules and only collect repos marked as configure diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java index c8ab1521a1669c..8fd593546ff7a6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java @@ -16,7 +16,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; import com.google.common.eventbus.Subscribe; @@ -31,10 +30,12 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.skyframe.MemoizingEvaluator; import java.io.IOException; import java.util.Arrays; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nullable; /** @@ -43,15 +44,15 @@ */ public class BazelLockFileModule extends BlazeModule { + private MemoizingEvaluator evaluator; private Path workspaceRoot; @Nullable private BazelModuleResolutionEvent moduleResolutionEvent; - private final Map - extensionResolutionEventsMap = new HashMap<>(); private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @Override public void beforeCommand(CommandEnvironment env) { + evaluator = env.getSkyframeExecutor().getEvaluator(); workspaceRoot = env.getWorkspace(); RepositoryOptions options = env.getOptions().getOptions(RepositoryOptions.class); if (options.lockfileMode.equals(LockfileMode.UPDATE)) { @@ -64,26 +65,38 @@ public void afterCommand() throws AbruptExitException { if (moduleResolutionEvent == null) { // Command does not use Bazel modules or the lockfile mode is not update. // Since Skyframe caches events, they are replayed even when nothing has changed. - Preconditions.checkState(extensionResolutionEventsMap.isEmpty()); return; } + // All nodes corresponding to module extensions that have been evaluated in the current build + // are done at this point. Look up entries by eval keys to record results even if validation + // later fails due to invalid imports. + // Note: This also picks up up-to-date result from previous builds that are not in the + // transitive closure of the current build. Since extension are potentially costly to evaluate, + // this is seen as an advantage. Full reproducibility can be ensured by running 'bazel shutdown' + // first if needed. + Map newExtensionInfos = + new ConcurrentHashMap<>(); + evaluator + .getInMemoryGraph() + .parallelForEach( + entry -> { + if (entry.isDone() + && entry.getKey() instanceof SingleExtensionValue.EvalKey key + // entry.getValue() can be null if the extension evaluation failed. + && entry.getValue() instanceof SingleExtensionValue value) { + newExtensionInfos.put(key.argument(), value.getLockFileInfo().get()); + } + }); + BazelLockFileValue oldLockfile = moduleResolutionEvent.getOnDiskLockfileValue(); - BazelLockFileValue newLockfile; - try { - // Create an updated version of the lockfile, keeping only the extension results from the old - // lockfile that are still up-to-date and adding the newly resolved extension results. - newLockfile = - moduleResolutionEvent.getResolutionOnlyLockfileValue().toBuilder() - .setModuleExtensions(combineModuleExtensions(oldLockfile)) - .build(); - } catch (ExternalDepsException e) { - logger.atSevere().withCause(e).log( - "Failed to read and parse the MODULE.bazel.lock file with error: %s." - + " Try deleting it and rerun the build.", - e.getMessage()); - return; - } + // Create an updated version of the lockfile, keeping only the extension results from the old + // lockfile that are still up-to-date and adding the newly resolved extension results. + BazelLockFileValue newLockfile = + moduleResolutionEvent.getResolutionOnlyLockfileValue().toBuilder() + .setModuleExtensions( + combineModuleExtensions(oldLockfile.getModuleExtensions(), newExtensionInfos)) + .build(); // Write the new value to the file, but only if needed. This is not just a performance // optimization: whenever the lockfile is updated, most Skyframe nodes will be marked as dirty @@ -93,7 +106,6 @@ public void afterCommand() throws AbruptExitException { updateLockfile(workspaceRoot, newLockfile); } this.moduleResolutionEvent = null; - this.extensionResolutionEventsMap.clear(); } /** @@ -102,12 +114,17 @@ public void afterCommand() throws AbruptExitException { */ private ImmutableMap< ModuleExtensionId, ImmutableMap> - combineModuleExtensions(BazelLockFileValue oldLockfile) throws ExternalDepsException { + combineModuleExtensions( + ImmutableMap< + ModuleExtensionId, + ImmutableMap> + oldExtensionInfos, + Map newExtensionInfos) { Map> updatedExtensionMap = new HashMap<>(); // Keep old extensions if they are still valid. - for (var entry : oldLockfile.getModuleExtensions().entrySet()) { + for (var entry : oldExtensionInfos.entrySet()) { var moduleExtensionId = entry.getKey(); var factorToLockedExtension = entry.getValue(); ModuleExtensionEvalFactors firstEntryFactors = @@ -117,32 +134,36 @@ public void afterCommand() throws AbruptExitException { // All entries for a single extension share the same usages digest, so it suffices to check // the first entry. if (shouldKeepExtension( - moduleExtensionId, firstEntryFactors, firstEntryExtension.getUsagesDigest())) { + moduleExtensionId, + firstEntryFactors, + firstEntryExtension.getUsagesDigest(), + newExtensionInfos.get(moduleExtensionId))) { updatedExtensionMap.put(moduleExtensionId, factorToLockedExtension); } } // Add the new resolved extensions - for (var event : extensionResolutionEventsMap.values()) { - LockFileModuleExtension extension = event.getModuleExtension(); + for (var extensionIdAndInfo : newExtensionInfos.entrySet()) { + LockFileModuleExtension extension = extensionIdAndInfo.getValue().moduleExtension(); if (!extension.shouldLockExtension()) { continue; } - var oldExtensionEntries = updatedExtensionMap.get(event.getExtensionId()); + var oldExtensionEntries = updatedExtensionMap.get(extensionIdAndInfo.getKey()); ImmutableMap extensionEntries; + var factors = extensionIdAndInfo.getValue().extensionFactors(); if (oldExtensionEntries != null) { // extension exists, add the new entry to the existing map extensionEntries = new ImmutableMap.Builder() .putAll(oldExtensionEntries) - .put(event.getExtensionFactors(), extension) + .put(factors, extension) .buildKeepingLast(); } else { // new extension - extensionEntries = ImmutableMap.of(event.getExtensionFactors(), extension); + extensionEntries = ImmutableMap.of(factors, extension); } - updatedExtensionMap.put(event.getExtensionId(), extensionEntries); + updatedExtensionMap.put(extensionIdAndInfo.getKey(), extensionEntries); } // The order in which extensions are added to extensionResolutionEvents depends on the order @@ -167,17 +188,18 @@ public void afterCommand() throws AbruptExitException { private boolean shouldKeepExtension( ModuleExtensionId extensionId, ModuleExtensionEvalFactors lockedExtensionKey, - byte[] oldUsagesDigest) { + byte[] oldUsagesDigest, + @Nullable LockFileModuleExtension.WithFactors newExtensionInfo) { // If there is a new event for this extension, compare it with the existing ones - ModuleExtensionResolutionEvent extEvent = extensionResolutionEventsMap.get(extensionId); - if (extEvent != null) { - boolean doNotLockExtension = !extEvent.getModuleExtension().shouldLockExtension(); + if (newExtensionInfo != null) { + boolean doNotLockExtension = !newExtensionInfo.moduleExtension().shouldLockExtension(); boolean dependencyOnOsChanged = - lockedExtensionKey.getOs().isEmpty() != extEvent.getExtensionFactors().getOs().isEmpty(); + lockedExtensionKey.getOs().isEmpty() + != newExtensionInfo.extensionFactors().getOs().isEmpty(); boolean dependencyOnArchChanged = lockedExtensionKey.getArch().isEmpty() - != extEvent.getExtensionFactors().getArch().isEmpty(); + != newExtensionInfo.extensionFactors().getArch().isEmpty(); if (doNotLockExtension || dependencyOnOsChanged || dependencyOnArchChanged) { return false; } @@ -228,10 +250,4 @@ public void bazelModuleResolved(BazelModuleResolutionEvent moduleResolutionEvent // sent after the command has modified the module file. this.moduleResolutionEvent = moduleResolutionEvent; } - - @Subscribe - public void moduleExtensionResolved(ModuleExtensionResolutionEvent extensionResolutionEvent) { - this.extensionResolutionEventsMap.put( - extensionResolutionEvent.getExtensionId(), extensionResolutionEvent); - } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java index 7f9e18404c53b2..89cd8b4d708a1d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java @@ -21,6 +21,7 @@ import static com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction.MODULE_OVERRIDES; import static com.google.devtools.build.lib.skyframe.PrecomputedValue.STARLARK_SEMANTICS; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue; @@ -29,7 +30,6 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; -import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; @@ -41,9 +41,8 @@ import net.starlark.java.eval.EvalException; /** - * Computes all information required for the {@code bazel mod tidy} command. The evaluation of all - * module extensions used by the root module is triggered to, as a side effect, emit any {@link - * RootModuleFileFixupEvent}s. + * Computes all information required for the {@code bazel mod tidy} command, which in particular + * requires evaluating all module extensions used by the root module. */ public class BazelModTidyFunction implements SkyFunction { @@ -94,29 +93,26 @@ public SkyValue compute(SkyKey skyKey, Environment env) .getOrDefault(ModuleKey.ROOT, ImmutableMap.of()) .keySet() .stream() - .map(SingleExtensionEvalValue::key) + // Use the eval-only key to avoid errors caused by incorrect imports - we can fix them. + .map(SingleExtensionValue::evalKey) .collect(toImmutableSet()); SkyframeLookupResult result = env.getValuesAndExceptions(extensionsUsedByRootModule); if (env.valuesMissing()) { return null; } + ImmutableList.Builder fixups = ImmutableList.builder(); for (SkyKey extension : extensionsUsedByRootModule) { - try { - result.getOrThrow(extension, ExternalDepsException.class); - } catch (ExternalDepsException e) { - if (e.getDetailedExitCode().getFailureDetail() == null - || !e.getDetailedExitCode() - .getFailureDetail() - .getExternalDeps() - .getCode() - .equals(FailureDetails.ExternalDeps.Code.INVALID_EXTENSION_IMPORT)) { - throw new BazelModTidyFunctionException(e, SkyFunctionException.Transience.PERSISTENT); - } - // This is an error bazel mod tidy can fix, so don't fail. + SkyValue value = result.get(extension); + if (value == null) { + return null; + } + if (result.get(extension) instanceof SingleExtensionValue evalValue) { + evalValue.getFixup().ifPresent(fixups::add); } } return BazelModTidyValue.create( + fixups.build(), buildozer.asPath(), rootModuleFileValue.getIncludeLabelToCompiledModuleFile(), MODULE_OVERRIDES.get(env), @@ -124,11 +120,4 @@ public SkyValue compute(SkyKey skyKey, Environment env) LOCKFILE_MODE.get(env), STARLARK_SEMANTICS.get(env)); } - - static final class BazelModTidyFunctionException extends SkyFunctionException { - - BazelModTidyFunctionException(ExternalDepsException cause, Transience transience) { - super(cause, transience); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java index cfd34f1fa3de7e..492d47c0eefa81 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.skyframe.SkyFunctions; @@ -23,6 +24,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.List; import java.util.Map; import net.starlark.java.eval.StarlarkSemantics; @@ -32,6 +34,9 @@ public abstract class BazelModTidyValue implements SkyValue { @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_MOD_TIDY; + /** Buildozer fixups for incorrect use_repo declarations by the root module. */ + public abstract ImmutableList fixups(); + /** The path of the buildozer binary provided by the "buildozer" module. */ public abstract Path buildozer(); @@ -53,6 +58,7 @@ public abstract class BazelModTidyValue implements SkyValue { public abstract StarlarkSemantics starlarkSemantics(); static BazelModTidyValue create( + List fixups, Path buildozer, ImmutableMap includeLabelToCompiledModuleFile, Map moduleOverrides, @@ -60,6 +66,7 @@ static BazelModTidyValue create( LockfileMode lockfileMode, StarlarkSemantics starlarkSemantics) { return new AutoValue_BazelModTidyValue( + ImmutableList.copyOf(fixups), buildozer, includeLabelToCompiledModuleFile, ImmutableMap.copyOf(moduleOverrides), diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java index e7bb90791bb087..9dc9e1b95379f6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java @@ -179,20 +179,20 @@ private ImmutableSetMultimap computeExtensionToRepoIn BazelDepGraphValue depGraphValue, Environment env) throws InterruptedException { ImmutableSet extensionEvalKeys = depGraphValue.getExtensionUsagesTable().rowKeySet(); - ImmutableList singleEvalKeys = - extensionEvalKeys.stream().map(SingleExtensionEvalValue::key).collect(toImmutableList()); - SkyframeLookupResult singleEvalValues = env.getValuesAndExceptions(singleEvalKeys); + ImmutableList singleExtensionKeys = + extensionEvalKeys.stream().map(SingleExtensionValue::key).collect(toImmutableList()); + SkyframeLookupResult singleExtensionValues = env.getValuesAndExceptions(singleExtensionKeys); ImmutableSetMultimap.Builder extensionToRepoInternalNames = ImmutableSetMultimap.builder(); - for (SingleExtensionEvalValue.Key singleEvalKey : singleEvalKeys) { - SingleExtensionEvalValue singleEvalValue = - (SingleExtensionEvalValue) singleEvalValues.get(singleEvalKey); - if (singleEvalValue == null) { + for (SingleExtensionValue.Key singleExtensionKey : singleExtensionKeys) { + SingleExtensionValue singleExtensionValue = + (SingleExtensionValue) singleExtensionValues.get(singleExtensionKey); + if (singleExtensionValue == null) { return null; } extensionToRepoInternalNames.putAll( - singleEvalKey.argument(), singleEvalValue.getGeneratedRepoSpecs().keySet()); + singleExtensionKey.argument(), singleExtensionValue.getGeneratedRepoSpecs().keySet()); } return extensionToRepoInternalNames.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 2af40858371f4b..e583bc30061adc 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -89,4 +89,11 @@ public abstract Builder setRecordedRepoMappingEntries( public abstract LockFileModuleExtension build(); } + + /** + * A {@link LockFileModuleExtension} together with its {@link ModuleExtensionEvalFactors}, + * comprising a single lockfile entry for a certain extension. + */ + public record WithFactors( + ModuleExtensionEvalFactors extensionFactors, LockFileModuleExtension moduleExtension) {} } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index 2f18222a89686a..ebc1c05324672b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -19,7 +19,6 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; @@ -27,8 +26,7 @@ import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.events.ExtendedEventHandler; -import com.google.devtools.build.lib.events.Reportable; +import com.google.devtools.build.lib.events.EventHandler; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; import java.util.ArrayList; import java.util.Collection; @@ -173,14 +171,9 @@ static ModuleExtensionMetadata create( reproducible); } - public void evaluate( - Collection usages, Set allRepos, ExtendedEventHandler handler) + public Optional generateFixup( + Collection usages, Set allRepos, EventHandler eventHandler) throws EvalException { - generateFixupMessage(usages, allRepos).forEach(reportable -> reportable.reportTo(handler)); - } - - private ImmutableList generateFixupMessage( - Collection usages, Set allRepos) throws EvalException { var rootUsages = usages.stream() .filter(usage -> usage.getUsingModule().equals(ModuleKey.ROOT)) @@ -188,7 +181,7 @@ private ImmutableList generateFixupMessage( if (rootUsages.isEmpty()) { // The root module doesn't use the current extension. Do not suggest fixes as the user isn't // expected to modify any other module's MODULE.bazel file. - return ImmutableList.of(); + return Optional.empty(); } // Every module only has at most a single usage of a given extension. ModuleExtensionUsage rootUsage = Iterables.getOnlyElement(rootUsages); @@ -196,7 +189,7 @@ private ImmutableList generateFixupMessage( var rootModuleDirectDevDeps = getRootModuleDirectDevDeps(allRepos); var rootModuleDirectDeps = getRootModuleDirectDeps(allRepos); if (rootModuleDirectDevDeps.isEmpty() && rootModuleDirectDeps.isEmpty()) { - return ImmutableList.of(); + return Optional.empty(); } Preconditions.checkState( rootModuleDirectDevDeps.isPresent() && rootModuleDirectDeps.isPresent()); @@ -212,15 +205,20 @@ private ImmutableList generateFixupMessage( + "usages with dev_dependency = True"); } - return generateFixupMessage( - rootUsage, allRepos, rootModuleDirectDeps.get(), rootModuleDirectDevDeps.get()); + return generateFixup( + rootUsage, + allRepos, + rootModuleDirectDeps.get(), + rootModuleDirectDevDeps.get(), + eventHandler); } - private static ImmutableList generateFixupMessage( + private static Optional generateFixup( ModuleExtensionUsage rootUsage, Set allRepos, Set expectedImports, - Set expectedDevImports) { + Set expectedDevImports, + EventHandler eventHandler) { var actualDevImports = ImmutableSet.copyOf(rootUsage.getDevImports()); var actualImports = rootUsage.getImports().values().stream() @@ -243,7 +241,7 @@ private static ImmutableList generateFixupMessage( && importsToRemove.isEmpty() && devImportsToAdd.isEmpty() && devImportsToRemove.isEmpty()) { - return ImmutableList.of(); + return Optional.empty(); } var message = @@ -343,8 +341,8 @@ private static ImmutableList generateFixupMessage( .flatMap(Optional::stream) .collect(toImmutableList()); - return ImmutableList.of( - Event.warn(location, message), new RootModuleFileFixupEvent(buildozerCommands, rootUsage)); + eventHandler.handle(Event.warn(location, message)); + return Optional.of(new RootModuleFileFixup(buildozerCommands, rootUsage)); } private static Optional makeUseRepoCommand( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java index fb1e9adcea3645..f128ae882df9e2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java @@ -42,7 +42,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept var moduleExtensionId = ((ModuleExtensionRepoMappingEntriesValue.Key) skyKey).argument(); var extensionEvalValue = - (SingleExtensionEvalValue) env.getValue(SingleExtensionEvalValue.key(moduleExtensionId)); + (SingleExtensionValue) env.getValue(SingleExtensionValue.key(moduleExtensionId)); if (extensionEvalValue == null) { return null; } @@ -57,7 +57,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept */ private ModuleExtensionRepoMappingEntriesValue computeRepoMappingEntries( ModuleExtensionId extensionId, - SingleExtensionEvalValue extensionEvalValue, + SingleExtensionValue extensionEvalValue, BazelDepGraphValue bazelDepGraphValue) { // Find the key of the module containing this extension. This will be used to compute additional // mappings -- any repo generated by an extension contained in the module "foo" can additionally diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java deleted file mode 100644 index 893608bef70afe..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java +++ /dev/null @@ -1,69 +0,0 @@ -// Copyright 2023 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.bazel.bzlmod; - -import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; - -/** - * After evaluating any module extension this event is sent from {@link SingleExtensionEvalFunction} - * holding the extension id and the resolution data LockFileModuleExtension. It will be received in - * {@link BazelLockFileModule} to be used to update the lockfile content. - * - *

Instances of this class are retained in Skyframe nodes and subject to frequent {@link - * java.util.Set}-based deduplication. As such, it must have a cheap implementation of {@link - * #hashCode()} and {@link #equals(Object)}. It currently uses reference equality since the logic - * that creates instances of this class already ensures that there is only one instance per - * extension id. - */ -public final class ModuleExtensionResolutionEvent implements Postable { - - private final ModuleExtensionId extensionId; - private final ModuleExtensionEvalFactors extensionFactors; - private final LockFileModuleExtension moduleExtension; - - private ModuleExtensionResolutionEvent( - ModuleExtensionId extensionId, - ModuleExtensionEvalFactors extensionFactors, - LockFileModuleExtension moduleExtension) { - this.extensionId = extensionId; - this.extensionFactors = extensionFactors; - this.moduleExtension = moduleExtension; - } - - public static ModuleExtensionResolutionEvent create( - ModuleExtensionId extensionId, - ModuleExtensionEvalFactors extensionFactors, - LockFileModuleExtension lockfileModuleExtension) { - return new ModuleExtensionResolutionEvent( - extensionId, extensionFactors, lockfileModuleExtension); - } - - public ModuleExtensionId getExtensionId() { - return extensionId; - } - - public ModuleExtensionEvalFactors getExtensionFactors() { - return extensionFactors; - } - - public LockFileModuleExtension getModuleExtension() { - return moduleExtension; - } - - @Override - public boolean storeForReplay() { - return true; - } -} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixupEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java similarity index 61% rename from src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixupEvent.java rename to src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java index 6163127c2bfb62..4182219aa2a77b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixupEvent.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java @@ -16,27 +16,17 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.events.ExtendedEventHandler; -import java.util.List; /** * Generated when incorrect use_repo calls are detected in the root module file according to {@link - * ModuleExtensionMetadata}, this event contains the buildozer commands required to bring the root - * module file into the expected state. + * ModuleExtensionMetadata} and contains the buildozer commands required to bring the root module + * file into the expected state. + * + * @param buildozerCommands The buildozer commands required to bring the root module file into the + * expected state. */ -public final class RootModuleFileFixupEvent implements ExtendedEventHandler.Postable { - private final ImmutableList buildozerCommands; - private final ModuleExtensionUsage usage; - - public RootModuleFileFixupEvent(List buildozerCommands, ModuleExtensionUsage usage) { - this.buildozerCommands = ImmutableList.copyOf(buildozerCommands); - this.usage = usage; - } - - /** The buildozer commands required to bring the root module file into the expected state. */ - public ImmutableList getBuildozerCommands() { - return buildozerCommands; - } +public record RootModuleFileFixup( + ImmutableList buildozerCommands, ModuleExtensionUsage usage) { /** A human-readable message describing the fixup after it has been applied. */ public String getSuccessMessage() { @@ -50,9 +40,4 @@ public String getSuccessMessage() { key.getUsageExportedName(), extensionId)) .orElseGet(() -> String.format("Updated use_repo calls for %s", extensionId)); } - - @Override - public boolean storeForReplay() { - return true; - } } 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 6485e97f2d8d94..dacc947c398e23 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 @@ -161,11 +161,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) lockedExtensionMap == null ? null : lockedExtensionMap.get(extension.getEvalFactors()); if (lockedExtension != null) { try { - SingleExtensionEvalValue singleExtensionEvalValue = + SingleExtensionValue singleExtensionValue = tryGettingValueFromLockFile( - env, extensionId, extension, usagesValue, lockedExtension); - if (singleExtensionEvalValue != null) { - return singleExtensionEvalValue; + env, + extensionId, + extension, + usagesValue, + extension.getEvalFactors(), + lockedExtension); + if (singleExtensionValue != null) { + return singleExtensionValue; } } catch (NeedsSkyframeRestartException e) { return null; @@ -204,16 +209,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } + Optional lockFileInfo; // At this point the extension has been evaluated successfully, but SingleExtensionEvalFunction // may still fail if imported repositories were not generated. However, since imports do not // influence the evaluation of the extension and the validation also runs when the extension - // result is taken from the lockfile, we can already post the update event. This is necessary to - // prevent the extension from rerunning when only the imports change. + // result is taken from the lockfile, we can already populate the lockfile info. This is + // necessary to prevent the extension from rerunning when only the imports change. if (lockfileMode.equals(LockfileMode.UPDATE)) { - env.getListener() - .post( - ModuleExtensionResolutionEvent.create( - extensionId, + lockFileInfo = + Optional.of( + new LockFileModuleExtension.WithFactors( extension.getEvalFactors(), LockFileModuleExtension.builder() .setBzlTransitiveDigest(extension.getBzlTransitiveDigest()) @@ -229,9 +234,11 @@ public SkyValue compute(SkyKey skyKey, Environment env) .setRecordedRepoMappingEntries( moduleExtensionResult.getRecordedRepoMappingEntries()) .build())); + } else { + lockFileInfo = Optional.empty(); } - return validateAndCreateSingleExtensionEvalValue( - generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, env); + return createSingleExtensionValue( + generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, lockFileInfo, env); } /** @@ -242,11 +249,12 @@ public SkyValue compute(SkyKey skyKey, Environment env) * don't return {@code null} in this case! */ @Nullable - private SingleExtensionEvalValue tryGettingValueFromLockFile( + private SingleExtensionValue tryGettingValueFromLockFile( Environment env, ModuleExtensionId extensionId, RunnableExtension extension, SingleExtensionUsagesValue usagesValue, + ModuleExtensionEvalFactors evalFactors, LockFileModuleExtension lockedExtension) throws SingleExtensionEvalFunctionException, InterruptedException, @@ -298,11 +306,12 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( // ignored } if (!diffRecorder.anyDiffsDetected()) { - return validateAndCreateSingleExtensionEvalValue( + return createSingleExtensionValue( lockedExtension.getGeneratedRepoSpecs(), lockedExtension.getModuleExtensionMetadata(), extensionId, usagesValue, + Optional.of(new LockFileModuleExtension.WithFactors(evalFactors, lockedExtension)), env); } if (lockfileMode.equals(LockfileMode.ERROR)) { @@ -405,31 +414,32 @@ private static boolean didRecordedInputsChange( /** * Validates the result of the module extension evaluation against the declared imports, throwing - * an exception if validation fails, and returns a SingleExtensionEvalValue otherwise. + * an exception if validation fails, and returns a SingleExtensionValue otherwise. * *

Since extension evaluation does not depend on the declared imports, the result of the * evaluation of the extension implementation function can be reused and persisted in the lockfile * even if validation fails. */ - private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue( + private SingleExtensionValue createSingleExtensionValue( ImmutableMap generatedRepoSpecs, Optional moduleExtensionMetadata, ModuleExtensionId extensionId, SingleExtensionUsagesValue usagesValue, + Optional lockFileInfo, Environment env) throws SingleExtensionEvalFunctionException { - // Evaluate the metadata before failing on invalid imports so that fixup warning are still - // emitted in case of an error. + Optional fixup = Optional.empty(); if (moduleExtensionMetadata.isPresent()) { try { - // TODO: ModuleExtensionMetadata#evaluate should throw ExternalDepsException instead of + // TODO: ModuleExtensionMetadata#generateFixup should throw ExternalDepsException instead of // EvalException. - moduleExtensionMetadata - .get() - .evaluate( - usagesValue.getExtensionUsages().values(), - generatedRepoSpecs.keySet(), - env.getListener()); + fixup = + moduleExtensionMetadata + .get() + .generateFixup( + usagesValue.getExtensionUsages().values(), + generatedRepoSpecs.keySet(), + env.getListener()); } catch (EvalException e) { env.getListener().handle(Event.error(e.getMessageWithStack())); throw new SingleExtensionEvalFunctionException( @@ -442,27 +452,7 @@ private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue( } } - // Check that all imported repos have actually been generated. - for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { - for (Entry repoImport : usage.getImports().entrySet()) { - if (!generatedRepoSpecs.containsKey(repoImport.getValue())) { - throw new SingleExtensionEvalFunctionException( - ExternalDepsException.withMessage( - Code.INVALID_EXTENSION_IMPORT, - "module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet it" - + " is imported as \"%s\" in the usage at %s%s", - extensionId.getExtensionName(), - extensionId.getBzlFileLabel(), - repoImport.getValue(), - repoImport.getKey(), - usage.getLocation(), - SpellChecker.didYouMean(repoImport.getValue(), generatedRepoSpecs.keySet())), - Transience.PERSISTENT); - } - } - } - - return SingleExtensionEvalValue.create( + return SingleExtensionValue.create( generatedRepoSpecs, generatedRepoSpecs.keySet().stream() .collect( @@ -470,7 +460,9 @@ private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue( e -> RepositoryName.createUnvalidated( usagesValue.getExtensionUniqueName() + "~" + e), - Function.identity()))); + Function.identity())), + lockFileInfo, + fixup); } private BzlLoadValue loadBzlFile( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java new file mode 100644 index 00000000000000..a1f406063abf42 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java @@ -0,0 +1,81 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package com.google.devtools.build.lib.bazel.bzlmod; + +import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import java.util.Map.Entry; +import javax.annotation.Nullable; +import net.starlark.java.spelling.SpellChecker; + +/** + * Validates the result of {@link SingleExtensionEvalFunction}. This is done in a separate + * SkyFunction so that the unvalidated value can be cached, avoiding a re-evaluation of the + * extension, even if the `use_repo` imports provided by the user are incorrect. + */ +public class SingleExtensionFunction implements SkyFunction { + + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws InterruptedException, SingleExtensionFunctionException { + ModuleExtensionId extensionId = (ModuleExtensionId) skyKey.argument(); + SingleExtensionUsagesValue usagesValue = + (SingleExtensionUsagesValue) env.getValue(SingleExtensionUsagesValue.key(extensionId)); + if (usagesValue == null) { + return null; + } + SingleExtensionValue evalOnlyValue = + (SingleExtensionValue) env.getValue(SingleExtensionValue.evalKey(extensionId)); + if (evalOnlyValue == null) { + return null; + } + + // Check that all imported repos have actually been generated. + for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { + for (Entry repoImport : usage.getImports().entrySet()) { + if (!evalOnlyValue.getGeneratedRepoSpecs().containsKey(repoImport.getValue())) { + throw new SingleExtensionFunctionException( + ExternalDepsException.withMessage( + Code.INVALID_EXTENSION_IMPORT, + "module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet it" + + " is imported as \"%s\" in the usage at %s%s", + extensionId.getExtensionName(), + extensionId.getBzlFileLabel(), + repoImport.getValue(), + repoImport.getKey(), + usage.getLocation(), + SpellChecker.didYouMean( + repoImport.getValue(), evalOnlyValue.getGeneratedRepoSpecs().keySet())), + Transience.PERSISTENT); + } + } + } + + return evalOnlyValue; + } + + static final class SingleExtensionFunctionException extends SkyFunctionException { + + SingleExtensionFunctionException(ExternalDepsException cause, Transience transience) { + super(cause, transience); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionValue.java similarity index 55% rename from src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java rename to src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionValue.java index c74686258262c9..e65b29dda1d583 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionValue.java @@ -26,11 +26,12 @@ import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.Optional; /** The result of evaluating a single module extension (see {@link SingleExtensionEvalFunction}). */ @AutoCodec(explicitlyAllowClass = {Package.class}) @AutoValue -public abstract class SingleExtensionEvalValue implements SkyValue { +public abstract class SingleExtensionValue implements SkyValue { /** * Returns the repos generated by the extension. The key is the "internal name" (as specified by * the extension) of the repo, and the value is the the repo specs that defins the repository . @@ -43,12 +44,25 @@ public abstract class SingleExtensionEvalValue implements SkyValue { */ public abstract ImmutableBiMap getCanonicalRepoNameToInternalNames(); + /** + * Returns the information stored about the extension in the lockfile. Is empty if the lockfile + * mode is not UPDATE. + */ + public abstract Optional getLockFileInfo(); + + /** + * Returns the buildozer fixup for incorrect use_repo declarations by the root module (if any). + */ + public abstract Optional getFixup(); + @AutoCodec.Instantiator - public static SingleExtensionEvalValue create( + public static SingleExtensionValue create( ImmutableMap generatedRepoSpecs, - ImmutableBiMap canonicalRepoNameToInternalNames) { - return new AutoValue_SingleExtensionEvalValue( - generatedRepoSpecs, canonicalRepoNameToInternalNames); + ImmutableBiMap canonicalRepoNameToInternalNames, + Optional lockFileInfo, + Optional fixup) { + return new AutoValue_SingleExtensionValue( + generatedRepoSpecs, canonicalRepoNameToInternalNames, lockFileInfo, fixup); } public static Key key(ModuleExtensionId id) { @@ -56,14 +70,23 @@ public static Key key(ModuleExtensionId id) { } /** - * The {@link com.google.devtools.build.skyframe.SkyKey} of a {@link - * com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionEvalValue} + * Provides access to {@link SingleExtensionValue} without validating the imports for the + * repositories generated by the extension. This is only meant for special applications such as + * {@code bazel mod tidy}. + */ + static EvalKey evalKey(ModuleExtensionId id) { + return EvalKey.create(id); + } + + /** + * The {@link SkyKey} of a {@link SingleExtensionValue} containing the result of extension + * evaluation. */ @AutoCodec - public static class Key extends AbstractSkyKey { + public static final class Key extends AbstractSkyKey { private static final SkyKeyInterner interner = SkyKey.newInterner(); - protected Key(ModuleExtensionId arg) { + private Key(ModuleExtensionId arg) { super(arg); } @@ -74,7 +97,7 @@ static Key create(ModuleExtensionId arg) { @Override public SkyFunctionName functionName() { - return SkyFunctions.SINGLE_EXTENSION_EVAL; + return SkyFunctions.SINGLE_EXTENSION; } @Override @@ -82,4 +105,37 @@ public SkyKeyInterner getSkyKeyInterner() { return interner; } } + + /** + * The {@link SkyKey} of an {@link SingleExtensionValue} containing the unvalidated result + * of extension evaluation. + */ + @AutoCodec + static final class EvalKey extends AbstractSkyKey { + private static final SkyKeyInterner interner = SkyKey.newInterner(); + + private EvalKey(ModuleExtensionId arg) { + super(arg); + } + + private static EvalKey create(ModuleExtensionId arg) { + return interner.intern(new EvalKey(arg)); + } + + @VisibleForSerialization + @AutoCodec.Interner + static EvalKey intern(EvalKey key) { + return interner.intern(key); + } + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.SINGLE_EXTENSION_EVAL; + } + + @Override + public SkyKeyInterner getSkyKeyInterner() { + return interner; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/commands/BUILD index 09bb1dc34e828b..bd2243525de4d4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/commands/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/commands/BUILD @@ -34,10 +34,10 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension", - "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_file_fixup_event", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:root_module_file_fixup", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:tidy", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand", "//src/main/java/com/google/devtools/build/lib/bazel/repository", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java index 9a2d48987b293a..36320c5ecefaf5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java @@ -44,7 +44,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue; import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey; -import com.google.devtools.build.lib.bazel.bzlmod.RootModuleFileFixupEvent; +import com.google.devtools.build.lib.bazel.bzlmod.RootModuleFileFixup; import com.google.devtools.build.lib.bazel.bzlmod.modcommand.ExtensionArg; import com.google.devtools.build.lib.bazel.bzlmod.modcommand.ExtensionArg.ExtensionArgConverter; import com.google.devtools.build.lib.bazel.bzlmod.modcommand.InvalidArgumentException; @@ -94,7 +94,6 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.io.Writer; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; @@ -568,14 +567,8 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe } private static class TidyEventRecorder { - final List fixupEvents = new ArrayList<>(); @Nullable BazelModuleResolutionEvent bazelModuleResolutionEvent; - @Subscribe - public void fixupGenerated(RootModuleFileFixupEvent event) { - fixupEvents.add(event); - } - @Subscribe public void bazelModuleResolved(BazelModuleResolutionEvent event) { bazelModuleResolutionEvent = event; @@ -590,8 +583,8 @@ private BlazeCommandResult runTidy( .addArg(modTidyValue.buildozer().getPathString()) .addArgs( Stream.concat( - eventRecorder.fixupEvents.stream() - .map(RootModuleFileFixupEvent::getBuildozerCommands) + modTidyValue.fixups().stream() + .map(RootModuleFileFixup::buildozerCommands) .flatMap(Collection::stream), Stream.of("format")) .collect(toImmutableList())) @@ -615,7 +608,7 @@ private BlazeCommandResult runTidy( Code.BUILDOZER_FAILED); } - for (RootModuleFileFixupEvent fixupEvent : eventRecorder.fixupEvents) { + for (RootModuleFileFixup fixupEvent : modTidyValue.fixups()) { env.getReporter().handle(Event.info(fixupEvent.getSuccessMessage())); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java index 27a020f0433a37..da7d5d2ec9b052 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadCycleReporter.java @@ -51,7 +51,9 @@ public class BzlLoadCycleReporter implements CyclesReporter.SingleCycleReporter SkyFunctions.isSkyFunction(SkyFunctions.BZL_LOAD); private static final Predicate IS_BZLMOD_EXTENSION = - SkyFunctions.isSkyFunction(SkyFunctions.SINGLE_EXTENSION_EVAL); + Predicates.or( + SkyFunctions.isSkyFunction(SkyFunctions.SINGLE_EXTENSION), + SkyFunctions.isSkyFunction(SkyFunctions.SINGLE_EXTENSION_EVAL)); private static void requestRepoDefinitions( ExtendedEventHandler eventHandler, Iterable repos) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java index 6ed9f158038a7f..ab2bedefe7cb8a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoCycleReporter.java @@ -54,6 +54,9 @@ public class BzlmodRepoCycleReporter implements CyclesReporter.SingleCycleReport private static final Predicate IS_EXTENSION_IMPL = SkyFunctions.isSkyFunction(SkyFunctions.SINGLE_EXTENSION_EVAL); + private static final Predicate IS_EXTENSION_VALIDATION = + SkyFunctions.isSkyFunction(SkyFunctions.SINGLE_EXTENSION); + private static final Predicate IS_REPO_MAPPING = SkyFunctions.isSkyFunction(SkyFunctions.REPOSITORY_MAPPING); @@ -107,6 +110,7 @@ public boolean maybeReportCycle( IS_PACKAGE_LOOKUP, IS_REPO_RULE, IS_EXTENSION_IMPL, + IS_EXTENSION_VALIDATION, IS_BZL_LOAD, IS_CONTAINING_PACKAGE, IS_REPO_MAPPING, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index 9d965727f655b1..850bf379914f9b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -33,7 +33,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey; import com.google.devtools.build.lib.bazel.bzlmod.NonRegistryOverride; import com.google.devtools.build.lib.bazel.bzlmod.RepoSpec; -import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionEvalValue; +import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionValue; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryMapping; @@ -133,17 +133,17 @@ public SkyValue compute(SkyKey skyKey, Environment env) return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE; } - SingleExtensionEvalValue extensionEval = - (SingleExtensionEvalValue) env.getValue(SingleExtensionEvalValue.key(extensionId.get())); - if (extensionEval == null) { + SingleExtensionValue extensionValue = + (SingleExtensionValue) env.getValue(SingleExtensionValue.key(extensionId.get())); + if (extensionValue == null) { return null; } - String internalRepo = extensionEval.getCanonicalRepoNameToInternalNames().get(repositoryName); + String internalRepo = extensionValue.getCanonicalRepoNameToInternalNames().get(repositoryName); if (internalRepo == null) { return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE; } - RepoSpec extRepoSpec = extensionEval.getGeneratedRepoSpecs().get(internalRepo); + RepoSpec extRepoSpec = extensionValue.getGeneratedRepoSpecs().get(internalRepo); return createRuleFromSpec(extRepoSpec, repositoryName, starlarkSemantics, env); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 838673d2371828..12729b26469ce9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -148,10 +148,10 @@ public final class SkyFunctions { SkyFunctionName.createHermetic("BAZEL_MODULE_RESOLUTION"); public static final SkyFunctionName BAZEL_MODULE_INSPECTION = SkyFunctionName.createHermetic("BAZEL_MODULE_INSPECTION"); - public static final SkyFunctionName MODULE_EXTENSION_RESOLUTION = - SkyFunctionName.createHermetic("MODULE_EXTENSION_RESOLUTION"); public static final SkyFunctionName SINGLE_EXTENSION_USAGES = SkyFunctionName.createHermetic("SINGLE_EXTENSION_USAGES"); + public static final SkyFunctionName SINGLE_EXTENSION = + SkyFunctionName.createHermetic("SINGLE_EXTENSION"); public static final SkyFunctionName SINGLE_EXTENSION_EVAL = SkyFunctionName.createNonHermetic("SINGLE_EXTENSION_EVAL"); public static final SkyFunctionName BAZEL_DEP_GRAPH = diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index fbb528e63be351..3d5c0c19601ced 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.bazel.bzlmod.RegistryFunction; import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction; import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionEvalFunction; +import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionFunction; import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionUsagesFunction; import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsFunction; import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil; @@ -174,6 +175,7 @@ public ImmutableMap getSkyFunctions(BlazeDirectori .put(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction()) .put(SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace())) .put(SkyFunctions.BAZEL_MODULE_RESOLUTION, new BazelModuleResolutionFunction()) + .put(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) .put( SkyFunctions.SINGLE_EXTENSION_EVAL, new SingleExtensionEvalFunction(directories, ImmutableMap::of, downloadManager)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index afccd4815703e6..f8a248f1933d2a 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -39,17 +39,18 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection_impl", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension", - "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_file_fixup_event", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:registry", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution", "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl", + "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:root_module_file_fixup", "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages/semantics", @@ -69,6 +70,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", 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 9ade865f492b77..768efa3d879121 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 @@ -17,14 +17,12 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createModuleKey; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount; -import static java.util.Comparator.comparing; import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.eventbus.Subscribe; import com.google.common.hash.HashFunction; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.ThreadStateReceiver; @@ -97,11 +95,8 @@ import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; -import java.util.List; import java.util.Objects; import java.util.Optional; -import java.util.SortedSet; -import java.util.TreeSet; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -116,21 +111,6 @@ @RunWith(JUnit4.class) public class ModuleExtensionResolutionTest extends FoundationTestCase { - private static class EventRecorder { - // Keep in deterministic order even though events are posted in Skyframe evaluation order. - private final SortedSet fixupEvents = - new TreeSet<>(comparing(RootModuleFileFixupEvent::getSuccessMessage)); - - @Subscribe - public void onFixupEvent(RootModuleFileFixupEvent fixupEvent) { - fixupEvents.add(fixupEvent); - } - - public List fixupEvents() { - return ImmutableList.copyOf(fixupEvents); - } - } - private Path workspaceRoot; private Path modulesRoot; private MemoizingEvaluator evaluator; @@ -139,11 +119,9 @@ public List fixupEvents() { private RecordingDifferencer differencer; private final CyclesReporter cyclesReporter = new CyclesReporter(new BzlLoadCycleReporter(), new BzlmodRepoCycleReporter()); - private final EventRecorder eventRecorder = new EventRecorder(); @Before public void setup() throws Exception { - eventBus.register(eventRecorder); workspaceRoot = scratch.dir("/ws"); String bazelToolsPath = "/ws/embedded_tools"; scratch.file(bazelToolsPath + "/MODULE.bazel", "module(name = 'bazel_tools')"); @@ -186,8 +164,6 @@ public void setup() throws Exception { HashFunction hashFunction = fileSystem.getDigestFunction().getHashFunction(); DownloadManager downloadManager = Mockito.mock(DownloadManager.class); - SingleExtensionEvalFunction singleExtensionEvalFunction = - new SingleExtensionEvalFunction(directories, ImmutableMap::of, downloadManager); StarlarkRepositoryFunction starlarkRepositoryFunction = new StarlarkRepositoryFunction(downloadManager); @@ -287,7 +263,10 @@ public void setup() throws Exception { .put(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction()) .put(SkyFunctions.BAZEL_MODULE_RESOLUTION, new BazelModuleResolutionFunction()) .put(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction()) - .put(SkyFunctions.SINGLE_EXTENSION_EVAL, singleExtensionEvalFunction) + .put(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) + .put( + SkyFunctions.SINGLE_EXTENSION_EVAL, + new SingleExtensionEvalFunction(directories, ImmutableMap::of, downloadManager)) .put(SkyFunctions.REGISTRY, new RegistryFunction(registryFactory)) .put(SkyFunctions.REPO_SPEC, new RepoSpecFunction()) .put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction()) @@ -1932,15 +1911,23 @@ public void extensionMetadata() throws Exception { + "\n" + "Fix the use_repo calls by running 'bazel mod tidy'.", ImmutableSet.of(EventKind.WARNING)); - assertThat(eventRecorder.fixupEvents()).hasSize(1); - assertThat(eventRecorder.fixupEvents().get(0).getBuildozerCommands()) + SingleExtensionValue evalValue = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); + assertThat(evalValue.getFixup()).isPresent(); + assertThat(evalValue.getFixup().get().buildozerCommands()) .containsExactly( "use_repo_add @ext//:defs.bzl ext missing_direct_dep non_dev_as_dev_dep", "use_repo_remove @ext//:defs.bzl ext dev_as_non_dev_dep indirect_dep invalid_dep", "use_repo_add dev @ext//:defs.bzl ext dev_as_non_dev_dep missing_direct_dev_dep", "use_repo_remove dev @ext//:defs.bzl ext indirect_dev_dep invalid_dev_dep" + " non_dev_as_dev_dep"); - assertThat(eventRecorder.fixupEvents().get(0).getSuccessMessage()) + assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2016,15 +2003,23 @@ public void extensionMetadata_all() throws Exception { + "\n" + "Fix the use_repo calls by running 'bazel mod tidy'.", ImmutableSet.of(EventKind.WARNING)); - assertThat(eventRecorder.fixupEvents()).hasSize(1); - assertThat(eventRecorder.fixupEvents().get(0).getBuildozerCommands()) + SingleExtensionValue evalValue = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); + assertThat(evalValue.getFixup()).isPresent(); + assertThat(evalValue.getFixup().get().buildozerCommands()) .containsExactly( "use_repo_add @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep missing_direct_dep" + " missing_direct_dev_dep", "use_repo_remove @ext//:defs.bzl ext invalid_dep", "use_repo_remove dev @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep" + " invalid_dev_dep"); - assertThat(eventRecorder.fixupEvents().get(0).getSuccessMessage()) + assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2102,14 +2097,22 @@ public void extensionMetadata_allDev() throws Exception { + "\n" + "Fix the use_repo calls by running 'bazel mod tidy'.", ImmutableSet.of(EventKind.WARNING)); - assertThat(eventRecorder.fixupEvents()).hasSize(1); - assertThat(eventRecorder.fixupEvents().get(0).getBuildozerCommands()) + SingleExtensionValue evalValue = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); + assertThat(evalValue.getFixup()).isPresent(); + assertThat(evalValue.getFixup().get().buildozerCommands()) .containsExactly( "use_repo_remove @ext//:defs.bzl ext direct_dep indirect_dep invalid_dep", "use_repo_add dev @ext//:defs.bzl ext direct_dep indirect_dep missing_direct_dep" + " missing_direct_dev_dep", "use_repo_remove dev @ext//:defs.bzl ext invalid_dev_dep"); - assertThat(eventRecorder.fixupEvents().get(0).getSuccessMessage()) + assertThat(evalValue.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for @ext//:defs.bzl%ext"); } @@ -2157,7 +2160,15 @@ public void extensionMetadata_noRootUsage() throws Exception { assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("indirect_dep_data"); assertEventCount(0, eventCollector); - assertThat(eventRecorder.fixupEvents()).isEmpty(); + SingleExtensionValue evalValue = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty()))); + assertThat(evalValue.getFixup()).isEmpty(); } @Test @@ -2235,15 +2246,38 @@ public void extensionMetadata_isolated() throws Exception { + "\n" + "Fix the use_repo calls by running 'bazel mod tidy'.", ImmutableSet.of(EventKind.WARNING)); - assertThat(eventRecorder.fixupEvents()).hasSize(2); - assertThat(eventRecorder.fixupEvents().get(0).getBuildozerCommands()) + SingleExtensionValue ext1Value = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), + "ext", + Optional.of( + ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext1"))))); + assertThat(ext1Value.getFixup()).isPresent(); + assertThat(ext1Value.getFixup().get().buildozerCommands()) .containsExactly( "use_repo_add ext1 direct_dep missing_direct_dep", "use_repo_remove ext1 indirect_dep"); - assertThat(eventRecorder.fixupEvents().get(0).getSuccessMessage()) + assertThat(ext1Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext1' of @ext//:defs.bzl%ext"); - assertThat(eventRecorder.fixupEvents().get(1).getBuildozerCommands()) + SingleExtensionValue ext2Value = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), + "ext", + Optional.of( + ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext2"))))); + assertThat(ext2Value.getFixup()).isPresent(); + assertThat(ext2Value.getFixup().get().buildozerCommands()) .containsExactly("use_repo_add ext2 missing_direct_dep"); - assertThat(eventRecorder.fixupEvents().get(1).getSuccessMessage()) + assertThat(ext2Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext2' of @ext//:defs.bzl%ext"); } @@ -2322,24 +2356,47 @@ public void extensionMetadata_isolatedDev() throws Exception { + "\n" + "Fix the use_repo calls by running 'bazel mod tidy'.", ImmutableSet.of(EventKind.WARNING)); - assertThat(eventRecorder.fixupEvents()).hasSize(2); - assertThat(eventRecorder.fixupEvents().get(0).getBuildozerCommands()) + SingleExtensionValue ext1Value = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), + "ext", + Optional.of( + ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext1"))))); + assertThat(ext1Value.getFixup()).isPresent(); + assertThat(ext1Value.getFixup().get().buildozerCommands()) .containsExactly( "use_repo_add ext1 direct_dep missing_direct_dep", "use_repo_remove ext1 indirect_dep"); - assertThat(eventRecorder.fixupEvents().get(0).getSuccessMessage()) + assertThat(ext1Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext1' of @ext//:defs.bzl%ext"); - assertThat(eventRecorder.fixupEvents().get(1).getBuildozerCommands()) + SingleExtensionValue ext2Value = + (SingleExtensionValue) + evaluator + .getDoneValues() + .get( + SingleExtensionValue.evalKey( + ModuleExtensionId.create( + Label.parseCanonical("@@ext~//:defs.bzl"), + "ext", + Optional.of( + ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, "ext2"))))); + assertThat(ext2Value.getFixup()).isPresent(); + assertThat(ext2Value.getFixup().get().buildozerCommands()) .containsExactly("use_repo_add ext2 missing_direct_dep"); - assertThat(eventRecorder.fixupEvents().get(1).getSuccessMessage()) + assertThat(ext2Value.getFixup().get().getSuccessMessage()) .isEqualTo("Updated use_repo calls for isolated usage 'ext2' of @ext//:defs.bzl%ext"); } - private EvaluationResult evaluateSimpleModuleExtension( + private EvaluationResult evaluateSimpleModuleExtension( String returnStatement) throws Exception { return evaluateSimpleModuleExtension(returnStatement, /* devDependency= */ false); } - private EvaluationResult evaluateSimpleModuleExtension( + private EvaluationResult evaluateSimpleModuleExtension( String returnStatement, boolean devDependency) throws Exception { String devDependencyStr = devDependency ? "True" : "False"; scratch.file( @@ -2360,7 +2417,7 @@ private EvaluationResult evaluateSimpleModuleExtension ModuleExtensionId.create(Label.parseCanonical("//:defs.bzl"), "ext", Optional.empty()); reporter.removeHandler(failFastHandler); return evaluator.evaluate( - ImmutableList.of(SingleExtensionEvalValue.key(extensionId)), evaluationContext); + ImmutableList.of(SingleExtensionValue.key(extensionId)), evaluationContext); } @Test @@ -2429,8 +2486,8 @@ public void printAndFailOnTag() throws Exception { ModuleExtensionId.create(Label.parseCanonical("//:defs.bzl"), "ext", Optional.empty()); reporter.removeHandler(failFastHandler); var result = - evaluator.evaluate( - ImmutableList.of(SingleExtensionEvalValue.key(extensionId)), evaluationContext); + evaluator.evaluate( + ImmutableList.of(SingleExtensionValue.key(extensionId)), evaluationContext); assertThat(result.hasError()).isTrue(); assertContainsEvent( diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index ef55cdd8c3444e..aad717fbc909ce 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -996,6 +996,114 @@ def testLockfileRecreatedAfterDeletion(self): self.assertEqual(old_data, new_data) + def testLockfileRecreationUsesPreviousBuildResults(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'lockfile_ext1 = use_extension("extension.bzl", "lockfile_ext1")', + 'use_repo(lockfile_ext1, "hello1")', + 'lockfile_ext2 = use_extension("extension.bzl", "lockfile_ext2")', + 'use_repo(lockfile_ext2, "hello2")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _module_ext1_impl(ctx):', + ' repo_rule(name="hello1")', + '', + 'lockfile_ext1 = module_extension(', + ' implementation=_module_ext1_impl,', + ')', + '', + 'def _module_ext2_impl(ctx):', + ' repo_rule(name="hello2")', + '', + 'lockfile_ext2 = module_extension(', + ' implementation=_module_ext2_impl,', + ')', + ], + ) + + self.RunBazel(['build', '@hello1//:all']) + # Return the lockfile to the state it had before the build: it didn't exist. + os.remove('MODULE.bazel.lock') + + # Perform an intermediate build that evaluates more extensions. + self.RunBazel(['build', '@hello1//:all', '@hello2//:all']) + with open('MODULE.bazel.lock', 'r') as lock_file: + old_data = lock_file.read() + # Return the lockfile to the state it had before the build: it didn't exist. + os.remove('MODULE.bazel.lock') + + # Rerun the original build with just ext1 and verify that extension results + # from intermediate builds are reused. + self.RunBazel(['build', '@hello1//:all']) + with open('MODULE.bazel.lock', 'r') as lock_file: + new_data = lock_file.read() + + self.assertEqual(old_data, new_data) + + def testLockfileRecreationOnlyUsesUpToDateBuildResults(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'lockfile_ext1 = use_extension("extension.bzl", "lockfile_ext1")', + 'use_repo(lockfile_ext1, "hello1")', + 'lockfile_ext2 = use_extension("extension.bzl", "lockfile_ext2")', + 'use_repo(lockfile_ext2, "hello2")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _module_ext1_impl(ctx):', + ' repo_rule(name="hello1")', + '', + 'lockfile_ext1 = module_extension(', + ' implementation=_module_ext1_impl,', + ')', + '', + 'def _module_ext2_impl(ctx):', + ' repo_rule(name="hello2")', + '', + 'lockfile_ext2 = module_extension(', + ' environ = ["FOO"], implementation=_module_ext2_impl,', + ')', + ], + ) + + self.RunBazel(['build', '@hello1//:all']) + with open('MODULE.bazel.lock', 'r') as lock_file: + old_data = lock_file.read() + # Return the lockfile to the state it had before the build: it didn't exist. + os.remove('MODULE.bazel.lock') + + # Perform an intermediate build that evaluates more extensions. + self.RunBazel(['build', '@hello1//:all', '@hello2//:all']) + # Return the lockfile to the state it had before the build: it didn't exist. + os.remove('MODULE.bazel.lock') + + # Rerun the original build with just ext1, but invalidate ext2. The + # intermediate build result should not be reused. + self.RunBazel(['build', '--repo_env=FOO=invalidate_ext2', '@hello1//:all']) + with open('MODULE.bazel.lock', 'r') as lock_file: + new_data = lock_file.read() + + self.assertEqual(old_data, new_data) + def testExtensionOsAndArch(self): self.ScratchFile( 'MODULE.bazel',