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 687aacf415c7ee..1742a31219c2bd 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 @@ -94,6 +94,8 @@ java_library( name = "bazel_lockfile_module", srcs = ["BazelLockFileModule.java"], deps = [ + ":common", + ":exception", ":resolution", ":resolution_impl", "//src/main/java/com/google/devtools/build/lib:runtime", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index 30597019640d22..ef7423d7d1ec75 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -128,6 +128,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) calculateUniqueNameForUsedExtensionId(extensionUsagesById); if (!lockfileMode.equals(LockfileMode.OFF)) { + // This will keep all module extension evaluation results, some of which may be stale due to + // changed usages. They will be removed in BazelLockFileModule. BazelLockFileValue updateLockfile = lockfile.toBuilder() .setLockFileVersion(BazelLockFileValue.LOCK_FILE_VERSION) 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 27113bc8dfc5d2..18cce4e953d979 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 @@ -18,6 +18,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Maps; import com.google.common.eventbus.Subscribe; import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions; @@ -67,25 +69,42 @@ public void afterCommand() throws AbruptExitException { RootedPath lockfilePath = RootedPath.toRootedPath(Root.fromPath(workspaceRoot), LabelConstants.MODULE_LOCKFILE_NAME); + // Read the existing lockfile (if none exists, will get an empty lockfile value) and get its + // module extension usages. This information is needed to determine which extension results are + // now stale and need to be removed. + BazelLockFileValue oldLockfile; + try { + oldLockfile = BazelLockFileFunction.getLockfileValue(lockfilePath); + } catch (IOException | JsonSyntaxException | NullPointerException 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; + } + ImmutableTable oldExtensionUsages; + try { + oldExtensionUsages = + BazelDepGraphFunction.getExtensionUsagesById(oldLockfile.getModuleDepGraph()); + } 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 with the events updates BazelLockFileValue lockfile; if (moduleResolutionEvent != null) { lockfile = moduleResolutionEvent.getLockfileValue(); } else { - // Read the existing lockfile (if none exists, will get an empty lockfile value) - try { - lockfile = BazelLockFileFunction.getLockfileValue(lockfilePath); - } catch (IOException | JsonSyntaxException | NullPointerException 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; - } + lockfile = oldLockfile; } lockfile = lockfile.toBuilder() - .setModuleExtensions(combineModuleExtensions(lockfile.getModuleExtensions())) + .setModuleExtensions( + combineModuleExtensions(lockfile.getModuleExtensions(), oldExtensionUsages)) .build(); // Write the new value to the file @@ -99,6 +118,7 @@ public void afterCommand() throws AbruptExitException { * extensions from the events (if any) * * @param oldModuleExtensions Module extensions stored in the current lockfile + * @param oldExtensionUsages Module extension usages stored in the current lockfile */ private ImmutableMap< ModuleExtensionId, ImmutableMap> @@ -106,7 +126,8 @@ public void afterCommand() throws AbruptExitException { ImmutableMap< ModuleExtensionId, ImmutableMap> - oldModuleExtensions) { + oldModuleExtensions, + ImmutableTable oldExtensionUsages) { Map> updatedExtensionMap = new HashMap<>(); @@ -115,7 +136,7 @@ public void afterCommand() throws AbruptExitException { oldModuleExtensions.forEach( (moduleExtensionId, innerMap) -> { ModuleExtensionEvalFactors firstEntryKey = innerMap.keySet().iterator().next(); - if (shouldKeepExtension(moduleExtensionId, firstEntryKey)) { + if (shouldKeepExtension(moduleExtensionId, firstEntryKey, oldExtensionUsages)) { updatedExtensionMap.put(moduleExtensionId, innerMap); } }); @@ -146,14 +167,21 @@ public void afterCommand() throws AbruptExitException { } /** - * Decide whether to keep this extension or not depending on both: 1. If its dependency on os & - * arch didn't change 2. If it is still has a usage in the module + * Decide whether to keep this extension or not depending on all of: + * + *
    + *
  1. If its dependency on os & arch didn't change + *
  2. If its usages haven't changed + *
* * @param lockedExtensionKey object holding the old extension id and state of os and arch + * @param oldExtensionUsages the usages of this extension in the existing lockfile * @return True if this extension should still be in lockfile, false otherwise */ private boolean shouldKeepExtension( - ModuleExtensionId extensionId, ModuleExtensionEvalFactors lockedExtensionKey) { + ModuleExtensionId extensionId, + ModuleExtensionEvalFactors lockedExtensionKey, + ImmutableTable oldExtensionUsages) { // If there is a new event for this extension, compare it with the existing ones ModuleExtensionResolutionEvent extEvent = extensionResolutionEventsMap.get(extensionId); @@ -168,11 +196,24 @@ private boolean shouldKeepExtension( } } - // If moduleResolutionEvent is null, then no usage has changed. So we don't need this check - if (moduleResolutionEvent != null) { - return moduleResolutionEvent.getExtensionUsagesById().containsRow(extensionId); + // If moduleResolutionEvent is null, then no usage has changed and all locked extension + // resolutions are still up-to-date. + if (moduleResolutionEvent == null) { + return true; } - return true; + // Otherwise, compare the current usages of this extension with the ones in the lockfile. We + // trim the usages to only the information that influences the evaluation of the extension so + // that irrelevant changes (e.g. locations or imports) don't cause the extension to be removed. + // Note: Extension results can still be stale for other reasons, e.g. because their transitive + // bzl hash changed, but such changes will be detected in SingleExtensionEvalFunction. + var currentTrimmedUsages = + Maps.transformValues( + moduleResolutionEvent.getExtensionUsagesById().row(extensionId), + ModuleExtensionUsage::trimForEvaluation); + var lockedTrimmedUsages = + Maps.transformValues( + oldExtensionUsages.row(extensionId), ModuleExtensionUsage::trimForEvaluation); + return currentTrimmedUsages.equals(lockedTrimmedUsages); } /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index 9d186432afef10..31657e5dee765d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; @@ -26,6 +28,8 @@ /** * Represents one usage of a module extension in one MODULE.bazel file. This class records all the * information pertinent to the proxy object returned from the {@code use_extension} call. + * + *

When adding new fields, make sure to update {@link #trimForEvaluation()} as well. */ @AutoValue @GenerateTypeAdapter @@ -86,6 +90,26 @@ public static Builder builder() { return new AutoValue_ModuleExtensionUsage.Builder(); } + /** + * Returns a new usage with all information removed that does not influence the evaluation of the + * extension. + */ + ModuleExtensionUsage trimForEvaluation() { + // We start with the full usage and selectively remove information that does not influence the + // evaluation of the extension. Compared to explicitly copying over the parts that do, this + // preserves correctness in case new fields are added without updating this code. + return toBuilder() + .setTags(getTags().stream().map(Tag::trimForEvaluation).collect(toImmutableList())) + // Locations are only used for error reporting and thus don't influence whether the + // evaluation of the extension is successful and what its result is in case of success. + .setLocation(Location.BUILTIN) + // Extension implementation functions do not see the imports, they are only validated + // against the set of generated repos in a validation step that comes afterward. + .setImports(ImmutableBiMap.of()) + .setDevImports(ImmutableSet.of()) + .build(); + } + /** Builder for {@link ModuleExtensionUsage}. */ @AutoValue.Builder public abstract static class Builder { 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 ab05d61cf821d2..6d4071c23e552d 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 @@ -18,14 +18,13 @@ import static com.google.common.base.StandardSystemProperty.OS_ARCH; import static com.google.common.collect.ImmutableBiMap.toImmutableBiMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.Maps.transformValues; import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; @@ -283,8 +282,13 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( boolean filesChanged = didFilesChange(env, lockedExtension.getAccumulatedFileDigests()); // Check extension data in lockfile is still valid, disregarding usage information that is not // relevant for the evaluation of the extension. - var trimmedLockedUsages = trimUsagesForEvaluation(lockedExtensionUsages); - var trimmedUsages = trimUsagesForEvaluation(usagesValue.getExtensionUsages()); + var trimmedLockedUsages = + ImmutableMap.copyOf( + transformValues(lockedExtensionUsages, ModuleExtensionUsage::trimForEvaluation)); + var trimmedUsages = + ImmutableMap.copyOf( + transformValues( + usagesValue.getExtensionUsages(), ModuleExtensionUsage::trimForEvaluation)); if (!filesChanged && Arrays.equals(bzlTransitiveDigest, lockedExtension.getBzlTransitiveDigest()) && trimmedUsages.equals(trimmedLockedUsages) @@ -422,33 +426,6 @@ private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue( Function.identity()))); } - /** - * Returns usages with all information removed that does not influence the evaluation of the - * extension. - */ - private static ImmutableMap trimUsagesForEvaluation( - Map usages) { - return ImmutableMap.copyOf( - Maps.transformValues( - usages, - usage -> - // We start with the full usage and selectively remove information that does not - // influence the evaluation of the extension. Compared to explicitly copying over - // the parts that do, this preserves correctness in case new fields are added to - // ModuleExtensionUsage without updating this code. - usage.toBuilder() - // Locations are only used for error reporting and thus don't influence whether - // the evaluation of the extension is successful and what its result is - // in case of success. - .setLocation(Location.BUILTIN) - // Extension implementation functions do not see the imports, they are only - // validated against the set of generated repos in a validation step that comes - // afterward. - .setImports(ImmutableBiMap.of()) - .setDevImports(ImmutableSet.of()) - .build())); - } - private BzlLoadValue loadBzlFile( Label bzlFileLabel, Location sampleUsageLocation, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java index 4ee4d6db356c92..b452e75d11f020 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java @@ -40,10 +40,27 @@ public abstract class Tag { /** The source location in the module file where this tag was created. */ public abstract Location getLocation(); + public abstract Builder toBuilder(); + public static Builder builder() { return new AutoValue_Tag.Builder(); } + /** + * Returns a new tag with all information removed that does not influence the evaluation of the + * extension defining the tag. + */ + Tag trimForEvaluation() { + // We start with the full usage and selectively remove information that does not influence the + // evaluation of the extension. Compared to explicitly copying over the parts that do, this + // preserves correctness in case new fields are added without updating this code. + return toBuilder() + // Locations are only used for error reporting and thus don't influence whether the + // evaluation of the extension is successful and what its result is in case of success. + .setLocation(Location.BUILTIN) + .build(); + } + /** Builder for {@link Tag}. */ @AutoValue.Builder public abstract static class Builder { diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index 7107b9277de8ba..7b618efec7eed5 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -1121,6 +1121,192 @@ def testExtensionOsAndArch(self): self.assertNotIn(added_key, extension_map) self.assertEqual(len(extension_map), 1) + def testExtensionEvaluationOnlyRerunOnRelevantUsagesChanges(self): + self.main_registry.createCcModule('aaa', '1.0') + + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext_1 = use_extension("extension.bzl", "ext_1")', + 'ext_1.tag()', + 'use_repo(ext_1, ext_1_dep = "dep")', + 'ext_2 = use_extension("extension.bzl", "ext_2")', + 'ext_2.tag()', + 'use_repo(ext_2, ext_2_dep = "dep")', + 'ext_3 = use_extension("extension.bzl", "ext_3")', + 'use_repo(ext_3, ext_3_dep = "dep")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("WORKSPACE")', + ' ctx.file("BUILD", "exports_files([\\"data.txt\\"])")', + ' ctx.file("data.txt", ctx.attr.value)', + ' print(ctx.attr.value)', + '', + 'repo_rule = repository_rule(', + ' implementation=_repo_rule_impl,', + ' attrs = {"value": attr.string()},)', + '', + 'def _ext_1_impl(ctx):', + ' print("Ext 1 is being evaluated")', + ( + ' num_tags = len([tag for mod in ctx.modules for tag in' + ' mod.tags.tag])' + ), + ' repo_rule(name="dep", value="Ext 1 saw %s tags" % num_tags)', + '', + 'ext_1 = module_extension(', + ' implementation=_ext_1_impl,', + ' tag_classes={"tag": tag_class()}', + ')', + '', + 'def _ext_2_impl(ctx):', + ' print("Ext 2 is being evaluated")', + ( + ' num_tags = len([tag for mod in ctx.modules for tag in' + ' mod.tags.tag])' + ), + ' repo_rule(name="dep", value="Ext 2 saw %s tags" % num_tags)', + '', + 'ext_2 = module_extension(', + ' implementation=_ext_2_impl,', + ' tag_classes={"tag": tag_class()}', + ')', + '', + 'def _ext_3_impl(ctx):', + ' print("Ext 3 is being evaluated")', + ( + ' num_tags = len([tag for mod in ctx.modules for tag in' + ' mod.tags.tag])' + ), + ' repo_rule(name="dep", value="Ext 3 saw %s tags" % num_tags)', + '', + 'ext_3 = module_extension(', + ' implementation=_ext_3_impl,', + ' tag_classes={"tag": tag_class()}', + ')', + ], + ) + + # Trigger evaluation of all extensions. + _, _, stderr = self.RunBazel( + ['build', '@ext_1_dep//:all', '@ext_2_dep//:all', '@ext_3_dep//:all'] + ) + stderr = '\n'.join(stderr) + + self.assertIn('Ext 1 is being evaluated', stderr) + self.assertIn('Ext 1 saw 1 tags', stderr) + self.assertIn('Ext 2 is being evaluated', stderr) + self.assertIn('Ext 2 saw 1 tags', stderr) + self.assertIn('Ext 3 is being evaluated', stderr) + self.assertIn('Ext 3 saw 0 tags', stderr) + ext_1_key = '//:extension.bzl%ext_1' + ext_2_key = '//:extension.bzl%ext_2' + ext_3_key = '//:extension.bzl%ext_3' + with open('MODULE.bazel.lock', 'r') as json_file: + lockfile = json.load(json_file) + self.assertIn(ext_1_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 1 saw 1 tags', + lockfile['moduleExtensions'][ext_1_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + self.assertIn(ext_2_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 2 saw 1 tags', + lockfile['moduleExtensions'][ext_2_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + self.assertIn(ext_3_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 3 saw 0 tags', + lockfile['moduleExtensions'][ext_3_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + + # Shut down bazel to empty the cache, modify the MODULE.bazel + # file in a way that does not affect the resolution of ext_1, + # but requires rerunning module resolution and removes ext_3, then + # trigger module resolution without evaluating any of the extensions. + self.RunBazel(['shutdown']) + self.ScratchFile( + 'MODULE.bazel', + [ + '# Added a dep to force rerunning module resolution.', + 'bazel_dep(name = "aaa", version = "1.0")', + ( + '# The usage of ext_1 is unchanged except for locations and' + ' imports.' + ), + 'ext_1 = use_extension("extension.bzl", "ext_1")', + 'ext_1.tag()', + 'use_repo(ext_1, ext_1_dep_new_name = "dep")', + '# The usage of ext_2 has a new tag.', + 'ext_2 = use_extension("extension.bzl", "ext_2")', + 'ext_2.tag()', + 'ext_2.tag()', + 'use_repo(ext_2, ext_2_dep = "dep")', + '# The usage of ext_3 has been removed.', + ], + ) + _, _, stderr = self.RunBazel(['build', '//:all']) + stderr = '\n'.join(stderr) + + self.assertNotIn('Ext 1 is being evaluated', stderr) + self.assertNotIn('Ext 2 is being evaluated', stderr) + self.assertNotIn('Ext 3 is being evaluated', stderr) + with open('MODULE.bazel.lock', 'r') as json_file: + lockfile = json.load(json_file) + # The usages of ext_1 did not change. + self.assertIn(ext_1_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 1 saw 1 tags', + lockfile['moduleExtensions'][ext_1_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + # The usages of ext_2 changed, but the extension is not re-evaluated, + # so its previous, now stale resolution result must have been removed. + self.assertNotIn(ext_2_key, lockfile['moduleExtensions']) + # The only usage of ext_3 was removed. + self.assertNotIn(ext_3_key, lockfile['moduleExtensions']) + + # Trigger evaluation of all remaining extensions. + _, _, stderr = self.RunBazel( + ['build', '@ext_1_dep_new_name//:all', '@ext_2_dep//:all'] + ) + stderr = '\n'.join(stderr) + + self.assertNotIn('Ext 1 is being evaluated', stderr) + self.assertIn('Ext 2 is being evaluated', stderr) + self.assertIn('Ext 2 saw 2 tags', stderr) + ext_1_key = '//:extension.bzl%ext_1' + ext_2_key = '//:extension.bzl%ext_2' + with open('MODULE.bazel.lock', 'r') as json_file: + lockfile = json.load(json_file) + self.assertIn(ext_1_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 1 saw 1 tags', + lockfile['moduleExtensions'][ext_1_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + self.assertIn(ext_2_key, lockfile['moduleExtensions']) + self.assertIn( + 'Ext 2 saw 2 tags', + lockfile['moduleExtensions'][ext_2_key]['general'][ + 'generatedRepoSpecs' + ]['dep']['attributes']['value'], + ) + self.assertNotIn(ext_3_key, lockfile['moduleExtensions']) + if __name__ == '__main__': absltest.main()