From 9bf8f396db5c8b204c61b34638ca15ece0328fc0 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 29 Sep 2023 16:11:40 -0700 Subject: [PATCH] Remove stale extension entries from lockfile When changes to module files result in module resolution rerunning, extension results from the lockfile may become stale due to changed usages even if the current build doesn't evaluate the affected extension. We now compare the current usages against the usages recorded in the lockfile and drop all extensions that became stale. Along the way, also trim tag locations when comparing usages for staleness detections as they do not affect the evaluation result. Closes #19658. PiperOrigin-RevId: 569612685 Change-Id: I8e9e30c37076ef2cbfa6e0498b9e003a078e4c67 --- .../devtools/build/lib/bazel/bzlmod/BUILD | 2 + .../bazel/bzlmod/BazelDepGraphFunction.java | 2 + .../lib/bazel/bzlmod/BazelLockFileModule.java | 81 ++++++-- .../bazel/bzlmod/ModuleExtensionUsage.java | 24 +++ .../bzlmod/SingleExtensionEvalFunction.java | 39 +--- .../devtools/build/lib/bazel/bzlmod/Tag.java | 17 ++ .../py/bazel/bzlmod/bazel_lockfile_test.py | 186 ++++++++++++++++++ 7 files changed, 300 insertions(+), 51 deletions(-) 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()