[7.0.0] Make lockfile extension updates more obviously correct (#20400)
* Removes code paths in `BazelLockFileModule` that are never taken now
that lockfile update events are replayed by Skyframe.
* `BazelModuleResolutionEvent` no longer contains a `BazelLockFileValue`
that is internally inconsistent: Previously, this values combined the
(potentially updated) extension usages resulting from the resolution
with the old locked extension results obtained from the previous state
of the lockfile. The logic in `BazelLockFileModule` handled this
correctly, but it should now be more obvious that it does so.
* No longer parse the lockfile twice, once in `BazelLockFileFunction`
and once in `BazelLockFileModule`.
Closes #20311.
Commit
https://github.com/bazelbuild/bazel/commit/1691f60c2ec222a392c8708ddc4cc35a969c4f7c
PiperOrigin-RevId: 586744057
Change-Id: Id85dbde627878c18baf3c375a0091a4f989b146b
Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
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 d07e293..d97a030 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
@@ -107,7 +107,6 @@
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:flogger",
- "//third_party:gson",
"//third_party:guava",
"//third_party:jsr305",
],
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 cc8c2f5..29f1a41 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,18 +128,17 @@
calculateUniqueNameForUsedExtensionId(extensionUsagesById);
if (lockfileMode.equals(LockfileMode.UPDATE)) {
- // 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)
+ BazelLockFileValue resolutionOnlyLockfile =
+ BazelLockFileValue.builder()
.setModuleFileHash(root.getModuleFileHash())
.setFlags(flags)
.setLocalOverrideHashes(localOverrideHashes)
.setModuleDepGraph(depGraph)
.build();
env.getListener()
- .post(BazelModuleResolutionEvent.create(updateLockfile, extensionUsagesById));
+ .post(
+ BazelModuleResolutionEvent.create(
+ lockfile, resolutionOnlyLockfile, extensionUsagesById));
}
return BazelDepGraphValue.create(
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 4036eb3..96fe6ac 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,6 +16,7 @@
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.collect.ImmutableTable;
@@ -31,7 +32,6 @@
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.gson.JsonSyntaxException;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
@@ -61,30 +61,22 @@
@Override
public void afterCommand() throws AbruptExitException {
- if (moduleResolutionEvent == null && extensionResolutionEventsMap.isEmpty()) {
- return; // nothing changed, do nothing!
- }
-
- 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());
+ 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;
}
- ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> oldExtensionUsages;
+
+ BazelLockFileValue oldLockfile = moduleResolutionEvent.getOnDiskLockfileValue();
+ BazelLockFileValue newLockfile;
try {
- oldExtensionUsages =
- BazelDepGraphFunction.getExtensionUsagesById(oldLockfile.getModuleDepGraph());
+ // 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."
@@ -93,25 +85,12 @@
return;
}
- // Create an updated version of the lockfile with the events updates
- BazelLockFileValue lockfile;
- if (moduleResolutionEvent != null) {
- lockfile = moduleResolutionEvent.getLockfileValue();
- } else {
- lockfile = oldLockfile;
- }
- lockfile =
- lockfile.toBuilder()
- .setModuleExtensions(
- combineModuleExtensions(lockfile.getModuleExtensions(), oldExtensionUsages))
- .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
// on the next build, which breaks commands such as `bazel config` that rely on
// com.google.devtools.build.skyframe.MemoizingEvaluator#getDoneValues.
- if (!lockfile.equals(oldLockfile)) {
- updateLockfile(lockfilePath, lockfile);
+ if (!newLockfile.equals(oldLockfile)) {
+ updateLockfile(workspaceRoot, newLockfile);
}
this.moduleResolutionEvent = null;
this.extensionResolutionEventsMap.clear();
@@ -120,30 +99,25 @@
/**
* Combines the old extensions stored in the lockfile -if they are still valid- with the new
* 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<ModuleExtensionEvalFactors, LockFileModuleExtension>>
- combineModuleExtensions(
- ImmutableMap<
- ModuleExtensionId,
- ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
- oldModuleExtensions,
- ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> oldExtensionUsages) {
-
+ combineModuleExtensions(BazelLockFileValue oldLockfile) throws ExternalDepsException {
Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
updatedExtensionMap = new HashMap<>();
- // Add old extensions if they are still valid
- oldModuleExtensions.forEach(
- (moduleExtensionId, innerMap) -> {
- ModuleExtensionEvalFactors firstEntryKey = innerMap.keySet().iterator().next();
- if (shouldKeepExtension(moduleExtensionId, firstEntryKey, oldExtensionUsages)) {
- updatedExtensionMap.put(moduleExtensionId, innerMap);
- }
- });
+ // Keep old extensions if they are still valid.
+ ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> oldExtensionUsages =
+ BazelDepGraphFunction.getExtensionUsagesById(oldLockfile.getModuleDepGraph());
+ for (var entry : oldLockfile.getModuleExtensions().entrySet()) {
+ var moduleExtensionId = entry.getKey();
+ var factorToLockedExtension = entry.getValue();
+ ModuleExtensionEvalFactors firstEntryFactors =
+ factorToLockedExtension.keySet().iterator().next();
+ if (shouldKeepExtension(moduleExtensionId, firstEntryFactors, oldExtensionUsages)) {
+ updatedExtensionMap.put(moduleExtensionId, factorToLockedExtension);
+ }
+ }
// Add the new resolved extensions
for (var event : extensionResolutionEventsMap.values()) {
@@ -200,11 +174,6 @@
}
}
- // If moduleResolutionEvent is null, then no usage has changed and all locked extension
- // resolutions are still up-to-date.
- if (moduleResolutionEvent == null) {
- 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.
@@ -218,10 +187,12 @@
/**
* Updates the data stored in the lockfile (MODULE.bazel.lock)
*
- * @param lockfilePath Rooted path to lockfile
+ * @param workspaceRoot Root of the workspace where the lockfile is located
* @param updatedLockfile The updated lockfile data to save
*/
- public static void updateLockfile(RootedPath lockfilePath, BazelLockFileValue updatedLockfile) {
+ public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updatedLockfile) {
+ RootedPath lockfilePath =
+ RootedPath.toRootedPath(Root.fromPath(workspaceRoot), LabelConstants.MODULE_LOCKFILE_NAME);
try {
FileSystemUtils.writeContent(
lockfilePath.asPath(),
@@ -249,5 +220,4 @@
this.extensionResolutionEventsMap.put(
extensionResolutionEvent.getExtensionId(), extensionResolutionEvent);
}
-
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java
index cd9c1e6..867eec90 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java
@@ -29,25 +29,39 @@
*/
public final class BazelModuleResolutionEvent implements Postable {
- private final BazelLockFileValue lockfileValue;
+ private final BazelLockFileValue onDiskLockfileValue;
+ private final BazelLockFileValue resolutionOnlyLockfileValue;
private final ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
extensionUsagesById;
private BazelModuleResolutionEvent(
- BazelLockFileValue lockfileValue,
+ BazelLockFileValue onDiskLockfileValue,
+ BazelLockFileValue resolutionOnlyLockfileValue,
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
- this.lockfileValue = lockfileValue;
+ this.onDiskLockfileValue = onDiskLockfileValue;
+ this.resolutionOnlyLockfileValue = resolutionOnlyLockfileValue;
this.extensionUsagesById = extensionUsagesById;
}
public static BazelModuleResolutionEvent create(
- BazelLockFileValue lockFileValue,
+ BazelLockFileValue onDiskLockfileValue,
+ BazelLockFileValue resolutionOnlyLockfileValue,
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
- return new BazelModuleResolutionEvent(lockFileValue, extensionUsagesById);
+ return new BazelModuleResolutionEvent(
+ onDiskLockfileValue, resolutionOnlyLockfileValue, extensionUsagesById);
}
- public BazelLockFileValue getLockfileValue() {
- return lockfileValue;
+ /** Returns the contents of the lockfile as it existed on disk before the current build. */
+ public BazelLockFileValue getOnDiskLockfileValue() {
+ return onDiskLockfileValue;
+ }
+
+ /**
+ * Returns the result of Bazel module resolution in the form of a lockfile without any information
+ * about module extension results.
+ */
+ public BazelLockFileValue getResolutionOnlyLockfileValue() {
+ return resolutionOnlyLockfileValue;
}
public ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunctionTest.java
index 54c00ba..f11407e 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunctionTest.java
@@ -34,7 +34,6 @@
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule;
import com.google.devtools.build.lib.clock.BlazeClock;
-import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction;
@@ -57,7 +56,6 @@
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileStateKey;
import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
@@ -183,11 +181,8 @@
if (localOverrideHashes == null) {
return null;
}
- RootedPath lockfilePath =
- RootedPath.toRootedPath(
- Root.fromPath(rootDirectory), LabelConstants.MODULE_LOCKFILE_NAME);
BazelLockFileModule.updateLockfile(
- lockfilePath,
+ rootDirectory,
BazelLockFileValue.builder()
.setModuleFileHash(key.moduleHash())
.setFlags(flags)
@@ -546,6 +541,7 @@
abstract static class UpdateLockFileKey implements SkyKey {
abstract String moduleHash();
+
abstract ImmutableMap<ModuleKey, Module> depGraph();
abstract ImmutableMap<String, ModuleOverride> overrides();
