Detect extension staleness due root module name/version change

Fixes #22121 Closes #22123. PiperOrigin-RevId: 630277746 Change-Id: I893a69b0048e933554f515684c6ab1fdae541c0b
bazelbuild · May 3, 2024 · 42423e0 · 42423e0
1 parent 963434a
commit 42423e0
Show file tree

Hide file tree

Showing 11 changed files with 229 additions and 90 deletions.
diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock
@@ -2917,7 +2917,7 @@
         "bzlTransitiveDigest": "tunTSmgwd2uvTzkCLtdbuCp0AI+WR+ftiPNqZ0rmcZk=",
         "recordedFileInputs": {
           "@@//MODULE.bazel": "eba5503742af5785c2d0d81d88e7407c7f23494b5162c055227435549b8774d1",
-          "@@//src/test/tools/bzlmod/MODULE.bazel.lock": "24f151a4b72d795d8290fe7b2f1a0a6d0a20110d644bc9d804ea881baadb8c52"
+          "@@//src/test/tools/bzlmod/MODULE.bazel.lock": "36d43264878997e1a777c4af38df4d4f17b0d7dcb37559e39bfb574ec2e33f42"
         },
         "recordedDirentsInputs": {},
         "envVariables": {},

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AbridgedModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AbridgedModule.java
@@ -16,12 +16,14 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import com.google.auto.value.AutoValue;
+import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
 
 /**
  * An abridged version of a {@link Module}, with a reduced set of information available, used for
  * module extension resolution.
  */
 @AutoValue
+@GenerateTypeAdapter
 public abstract class AbridgedModule {
   public abstract String getName();
 

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
@@ -104,6 +104,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//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",

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
@@ -25,14 +25,13 @@
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 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;
@@ -44,15 +43,15 @@
  */
 public class BazelLockFileModule extends BlazeModule {
 
-  private MemoizingEvaluator evaluator;
+  private SkyframeExecutor executor;
   private Path workspaceRoot;
   @Nullable private BazelModuleResolutionEvent moduleResolutionEvent;
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   @Override
   public void beforeCommand(CommandEnvironment env) {
-    evaluator = env.getSkyframeExecutor().getEvaluator();
+    executor = env.getSkyframeExecutor();
     workspaceRoot = env.getWorkspace();
     RepositoryOptions options = env.getOptions().getOptions(RepositoryOptions.class);
     if (options.lockfileMode.equals(LockfileMode.UPDATE)) {
@@ -77,7 +76,8 @@ public void afterCommand() throws AbruptExitException {
     // first if needed.
     Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos =
         new ConcurrentHashMap<>();
-    evaluator
+    executor
+        .getEvaluator()
         .getInMemoryGraph()
         .parallelForEach(
             entry -> {
@@ -89,13 +89,23 @@ public void afterCommand() throws AbruptExitException {
               }
             });
 
+    BazelDepGraphValue depGraphValue;
+    try {
+      depGraphValue =
+          (BazelDepGraphValue) executor.getEvaluator().getExistingValue(BazelDepGraphValue.KEY);
+    } catch (InterruptedException e) {
+      // Not thrown in Bazel.
+      throw new IllegalStateException(e);
+    }
+
     BazelLockFileValue oldLockfile = moduleResolutionEvent.getOnDiskLockfileValue();
     // 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))
+                combineModuleExtensions(
+                    oldLockfile.getModuleExtensions(), newExtensionInfos, depGraphValue))
             .build();
 
     // Write the new value to the file, but only if needed. This is not just a performance
@@ -119,7 +129,8 @@ public void afterCommand() throws AbruptExitException {
                   ModuleExtensionId,
                   ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
               oldExtensionInfos,
-          Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos) {
+          Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos,
+          BazelDepGraphValue depGraphValue) {
     Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
         updatedExtensionMap = new HashMap<>();
 
@@ -136,8 +147,8 @@ public void afterCommand() throws AbruptExitException {
       if (shouldKeepExtension(
           moduleExtensionId,
           firstEntryFactors,
-          firstEntryExtension.getUsagesDigest(),
-          newExtensionInfos.get(moduleExtensionId))) {
+          newExtensionInfos.get(moduleExtensionId),
+          depGraphValue)) {
         updatedExtensionMap.put(moduleExtensionId, factorToLockedExtension);
       }
     }
@@ -174,47 +185,46 @@ public void afterCommand() throws AbruptExitException {
   }
 
   /**
-   * Decide whether to keep this extension or not depending on all of:
+   * Keep an extension if and only if:
    *
    * <ol>
-   *   <li>If its dependency on os & arch didn't change
-   *   <li>If its usages haven't changed
+   *   <li>it still has usages
+   *   <li>its dependency on os & arch didn't change
+   *   <li>it hasn't become reproducible
    * </ol>
    *
+   * We do not check for changes in the usage hash of the extension or e.g. hashes of files accessed
+   * during the evaluation. These values are checked in SingleExtensionEvalFunction.
+   *
    * @param lockedExtensionKey object holding the old extension id and state of os and arch
-   * @param oldUsagesDigest the digest of usages of this extension in the existing lockfile
+   * @param newExtensionInfo the in-memory lockfile entry produced by the most recent up-to-date
+   *     evaluation of this extension (if any)
+   * @param depGraphValue the dep graph value
    * @return True if this extension should still be in lockfile, false otherwise
    */
   private boolean shouldKeepExtension(
-      ModuleExtensionId extensionId,
+      ModuleExtensionId moduleExtensionId,
       ModuleExtensionEvalFactors lockedExtensionKey,
-      byte[] oldUsagesDigest,
-      @Nullable LockFileModuleExtension.WithFactors newExtensionInfo) {
-
-    // If there is a new event for this extension, compare it with the existing ones
-    if (newExtensionInfo != null) {
-      boolean doNotLockExtension = !newExtensionInfo.moduleExtension().shouldLockExtension();
-      boolean dependencyOnOsChanged =
-          lockedExtensionKey.getOs().isEmpty()
-              != newExtensionInfo.extensionFactors().getOs().isEmpty();
-      boolean dependencyOnArchChanged =
-          lockedExtensionKey.getArch().isEmpty()
-              != newExtensionInfo.extensionFactors().getArch().isEmpty();
-      if (doNotLockExtension || dependencyOnOsChanged || dependencyOnArchChanged) {
-        return false;
-      }
+      @Nullable LockFileModuleExtension.WithFactors newExtensionInfo,
+      BazelDepGraphValue depGraphValue) {
+    if (!depGraphValue.getExtensionUsagesTable().containsRow(moduleExtensionId)) {
+      return false;
+    }
+
+    // If there is no new extension info, the properties of the existing extension entry can't have
+    // changed.
+    if (newExtensionInfo == 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.
-    // 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.
-    return Arrays.equals(
-        ModuleExtensionUsage.hashForEvaluation(
-            GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(),
-            moduleResolutionEvent.getExtensionUsagesById().row(extensionId)),
-        oldUsagesDigest);
+    boolean doNotLockExtension = !newExtensionInfo.moduleExtension().shouldLockExtension();
+    boolean dependencyOnOsChanged =
+        lockedExtensionKey.getOs().isEmpty()
+            != newExtensionInfo.extensionFactors().getOs().isEmpty();
+    boolean dependencyOnArchChanged =
+        lockedExtensionKey.getArch().isEmpty()
+            != newExtensionInfo.extensionFactors().getArch().isEmpty();
+    return !doNotLockExtension && !dependencyOnOsChanged && !dependencyOnArchChanged;
   }
 
   /**

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java
@@ -482,7 +482,7 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
         .create();
   }
 
-  public static Gson createModuleExtensionUsagesHashGson() {
+  public static Gson createSingleExtensionUsagesValueHashGson() {
     return newGsonBuilder().create();
   }
 

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
@@ -19,12 +19,8 @@
 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.Maps;
-import com.google.common.hash.Hashing;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import com.google.gson.Gson;
 import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
 import java.util.Optional;
 import net.starlark.java.syntax.Location;
@@ -94,22 +90,11 @@ public static Builder builder() {
     return new AutoValue_ModuleExtensionUsage.Builder();
   }
 
-  /**
-   * Turns the given collection of usages for a particular extension into a hash that can be
-   * compared for equality with another hash obtained in this way and compares equal only if the two
-   * original collections of usages are equivalent for the purpose of evaluating the extension.
-   */
-  static byte[] hashForEvaluation(Gson gson, ImmutableMap<ModuleKey, ModuleExtensionUsage> usages) {
-    ImmutableMap<ModuleKey, ModuleExtensionUsage> trimmedUsages =
-        ImmutableMap.copyOf(Maps.transformValues(usages, ModuleExtensionUsage::trimForEvaluation));
-    return Hashing.sha256().hashUnencodedChars(gson.toJson(trimmedUsages)).asBytes();
-  }
-
   /**
    * Returns a new usage with all information removed that does not influence the evaluation of the
    * extension.
    */
-  private ModuleExtensionUsage trimForEvaluation() {
+  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.

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
@@ -224,9 +224,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                   LockFileModuleExtension.builder()
                       .setBzlTransitiveDigest(extension.getBzlTransitiveDigest())
                       .setUsagesDigest(
-                          ModuleExtensionUsage.hashForEvaluation(
-                              GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(),
-                              usagesValue.getExtensionUsages()))
+                          SingleExtensionUsagesValue.hashForEvaluation(
+                              GsonTypeAdapterUtil.createSingleExtensionUsagesValueHashGson(),
+                              usagesValue))
                       .setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs())
                       .setRecordedDirentsInputs(moduleExtensionResult.getRecordedDirentsInputs())
                       .setEnvVariables(extension.getEnvVars())
@@ -281,9 +281,8 @@ private SingleExtensionValue tryGettingValueFromLockFile(
       // Check extension data in lockfile is still valid, disregarding usage information that is not
       // relevant for the evaluation of the extension.
       if (!Arrays.equals(
-          ModuleExtensionUsage.hashForEvaluation(
-              GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(),
-              usagesValue.getExtensionUsages()),
+          SingleExtensionUsagesValue.hashForEvaluation(
+              GsonTypeAdapterUtil.createSingleExtensionUsagesValueHashGson(), usagesValue),
           lockedExtension.getUsagesDigest())) {
         diffRecorder.record("The usages of the extension '" + extensionId + "' have changed");
       }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesFunction.java
@@ -49,6 +49,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     }
 
     ModuleExtensionId id = (ModuleExtensionId) skyKey.argument();
+    // We never request an extension without usages in Skyframe.
     ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> usagesTable =
         bazelDepGraphValue.getExtensionUsagesTable();
     return SingleExtensionUsagesValue.create(

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java
@@ -18,6 +18,8 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+import com.google.common.hash.Hashing;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
@@ -26,9 +28,17 @@
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import com.google.gson.Gson;
+import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
 
-/** The result of {@link SingleExtensionUsagesFunction}. */
+/**
+ * The result of {@link SingleExtensionUsagesFunction}.
+ *
+ * <p>When adding or exposing new fields to extensions, make sure to update {@link
+ * #trimForEvaluation()} as well.
+ */
 @AutoValue
+@GenerateTypeAdapter
 public abstract class SingleExtensionUsagesValue implements SkyValue {
   /** All usages of this extension, by the key of the module where the usage occurs. */
   // Note: Equality of SingleExtensionUsagesValue does not check for equality of the order of the
@@ -55,6 +65,35 @@ public static SingleExtensionUsagesValue create(
         extensionUsages, extensionUniqueName, abridgedModules, repoMappings);
   }
 
+  /**
+   * Turns the given usages value for a particular extension into a hash that can be compared for
+   * equality with another hash obtained in this way and compares equal only if the two values are
+   * equivalent for the purpose of evaluating the extension.
+   */
+  static byte[] hashForEvaluation(Gson gson, SingleExtensionUsagesValue usagesValue) {
+    return Hashing.sha256()
+        .hashUnencodedChars(gson.toJson(usagesValue.trimForEvaluation()))
+        .asBytes();
+  }
+
+  /**
+   * Returns a new value with only the information that influences the evaluation of the extension
+   * and isn't tracked elsewhere.
+   */
+  SingleExtensionUsagesValue trimForEvaluation() {
+    return SingleExtensionUsagesValue.create(
+        ImmutableMap.copyOf(
+            Maps.transformValues(getExtensionUsages(), ModuleExtensionUsage::trimForEvaluation)),
+        // extensionUniqueName: Not accessible to the extension's implementation function.
+        // TODO: Reconsider this when resolving #19055.
+        "",
+        getAbridgedModules(),
+        // repoMappings: The usage of repo mappings by the extension's implementation function is
+        // tracked on the level of individual entries and all label attributes are provided as
+        // `Label`, which exclusively reference canonical repository names.
+        ImmutableMap.of());
+  }
+
   public static Key key(ModuleExtensionId id) {
     return Key.create(id);
   }