[8.7.0] Fold environ into the predeclared inputs hash

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.gson.Gson;
 import com.google.gson.TypeAdapter;
 import com.google.gson.TypeAdapterFactory;
@@ -34,6 +35,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 import java.util.function.Function;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
@@ -71,6 +74,14 @@ private DelegateTypeAdapterFactory(
           raw -> new LinkedHashMap<>((Map<?, ?>) raw),
           delegate -> ImmutableMap.copyOf((Map<?, ?>) delegate));
 
+  public static final TypeAdapterFactory IMMUTABLE_SORTED_MAP =
+      new DelegateTypeAdapterFactory<>(
+          ImmutableSortedMap.class,
+          SortedMap.class,
+          TreeMap.class,
+          raw -> new TreeMap<>((SortedMap<?, ?>) raw),
+          delegate -> ImmutableSortedMap.copyOf((SortedMap<?, ?>) delegate));
+
   public static final TypeAdapterFactory IMMUTABLE_BIMAP =
       new DelegateTypeAdapterFactory<>(
           ImmutableBiMap.class,

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java

@@ -19,6 +19,7 @@
 import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_LIST;
 import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP;
 import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET;
+import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SORTED_MAP;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
@@ -469,6 +470,7 @@ private static GsonBuilder newGsonBuilder() {
         .registerTypeAdapterFactory(GenerateTypeAdapter.FACTORY)
         .registerTypeAdapterFactory(DICT)
         .registerTypeAdapterFactory(IMMUTABLE_MAP)
+        .registerTypeAdapterFactory(IMMUTABLE_SORTED_MAP)
         .registerTypeAdapterFactory(IMMUTABLE_LIST)
         .registerTypeAdapterFactory(IMMUTABLE_BIMAP)
         .registerTypeAdapterFactory(IMMUTABLE_SET)

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java

@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableTable;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
@@ -236,9 +237,9 @@ public RunModuleExtensionResult run(
               attributesValue));
     }
     return new RunModuleExtensionResult(
-        ImmutableMap.of(),
-        ImmutableMap.of(),
-        ImmutableMap.of(),
+        ImmutableSortedMap.of(),
+        ImmutableSortedMap.of(),
+        ImmutableSortedMap.of(),
         generatedRepoSpecs.buildOrThrow(),
         ModuleExtensionMetadata.REPRODUCIBLE,
         ImmutableTable.of());

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java

@@ -16,6 +16,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableTable;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
@@ -44,11 +45,11 @@ public static Builder builder() {
   @SuppressWarnings("mutable")
   public abstract byte[] getUsagesDigest();
 
-  public abstract ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs();
+  public abstract ImmutableSortedMap<RepoRecordedInput.File, String> getRecordedFileInputs();
 
-  public abstract ImmutableMap<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();
+  public abstract ImmutableSortedMap<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();
 
-  public abstract ImmutableMap<RepoRecordedInput.EnvVar, Optional<String>> getEnvVariables();
+  public abstract ImmutableSortedMap<RepoRecordedInput.EnvVar, Optional<String>> getEnvVariables();
 
   public abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();
 
@@ -72,13 +73,13 @@ public abstract static class Builder {
     public abstract Builder setUsagesDigest(byte[] digest);
 
     public abstract Builder setRecordedFileInputs(
-        ImmutableMap<RepoRecordedInput.File, String> value);
+        ImmutableSortedMap<RepoRecordedInput.File, String> value);
 
     public abstract Builder setRecordedDirentsInputs(
-        ImmutableMap<RepoRecordedInput.Dirents, String> value);
+        ImmutableSortedMap<RepoRecordedInput.Dirents, String> value);
 
     public abstract Builder setEnvVariables(
-        ImmutableMap<RepoRecordedInput.EnvVar, Optional<String>> value);
+        ImmutableSortedMap<RepoRecordedInput.EnvVar, Optional<String>> value);
 
     public abstract Builder setGeneratedRepoSpecs(ImmutableMap<String, RepoSpec> value);
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java

@@ -16,6 +16,7 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableTable;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -56,9 +57,9 @@ RunModuleExtensionResult run(
 
   /* Holds the result data from running a module extension */
   record RunModuleExtensionResult(
-      ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs,
-      ImmutableMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs,
-      ImmutableMap<RepoRecordedInput.EnvVar, Optional<String>> recordedEnvVarInputs,
+      ImmutableSortedMap<RepoRecordedInput.File, String> recordedFileInputs,
+      ImmutableSortedMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs,
+      ImmutableSortedMap<RepoRecordedInput.EnvVar, Optional<String>> recordedEnvVarInputs,
       ImmutableMap<String, RepoSpec> generatedRepoSpecs,
       ModuleExtensionMetadata moduleExtensionMetadata,
       ImmutableTable<RepositoryName, String, RepositoryName> recordedRepoMappingEntries) {}

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java

@@ -270,20 +270,20 @@ protected final void registerAsyncTask(AsyncTask task) {
   protected abstract boolean shouldDeleteWorkingDirectoryOnClose(boolean successful);
 
   /** Returns the file digests used by this context object so far. */
-  public ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs() {
+  public ImmutableSortedMap<RepoRecordedInput.File, String> getRecordedFileInputs() {
     return ImmutableSortedMap.copyOf(recordedFileInputs);
   }
 
-  public ImmutableMap<Dirents, String> getRecordedDirentsInputs() {
+  public ImmutableSortedMap<Dirents, String> getRecordedDirentsInputs() {
     return ImmutableSortedMap.copyOf(recordedDirentsInputs);
   }
 
-  public ImmutableMap<RepoRecordedInput.EnvVar, Optional<String>> getRecordedEnvVarInputs()
+  public ImmutableSortedMap<RepoRecordedInput.EnvVar, Optional<String>> getRecordedEnvVarInputs()
       throws InterruptedException {
     // getEnvVarValues doesn't return null since the Skyframe dependencies have already been
     // established by getenv calls.
     return RepoRecordedInput.EnvVar.wrap(
-        ImmutableSortedMap.copyOf(RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys)));
+        RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys));
   }
 
   protected void checkInOutputDirectory(String operation, StarlarkPath path)

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -19,6 +19,7 @@
 import com.github.difflib.patch.PatchFailedException;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent;
@@ -141,8 +142,8 @@ protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) {
     return !successful;
   }
 
-  public ImmutableMap<RepoRecordedInput.DirTree, String> getRecordedDirTreeInputs() {
-    return ImmutableMap.copyOf(recordedDirTreeInputs);
+  public ImmutableSortedMap<RepoRecordedInput.DirTree, String> getRecordedDirTreeInputs() {
+    return ImmutableSortedMap.copyOf(recordedDirTreeInputs);
   }
 
   @StarlarkMethod(

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java

@@ -329,8 +329,6 @@ private FetchResult fetchInternal(
 
       // Modify marker data to include the files/dirents/env vars used by the rule's implementation
       // function.
-      recordedInputValues.putAll(
-          Maps.transformValues(RepoRecordedInput.EnvVar.wrap(envVarValues), v -> v.orElse(null)));
       recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs());
       recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs());
       recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirTreeInputs());

src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java

@@ -15,13 +15,14 @@
 package com.google.devtools.build.lib.rules.repository;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
+import static java.util.Comparator.naturalOrder;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -545,10 +546,12 @@ public RepoRecordedInput parse(String s) {
 
     final String name;
 
-    public static ImmutableMap<EnvVar, Optional<String>> wrap(
+    public static ImmutableSortedMap<EnvVar, Optional<String>> wrap(
         Map<String, Optional<String>> envVars) {
       return envVars.entrySet().stream()
-          .collect(toImmutableMap(e -> new EnvVar(e.getKey()), Map.Entry::getValue));
+          .collect(
+              toImmutableSortedMap(
+                  naturalOrder(), e -> new EnvVar(e.getKey()), Map.Entry::getValue));
     }
 
     private EnvVar(String name) {

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -22,6 +22,8 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue;
@@ -62,6 +64,7 @@
 import java.io.IOException;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Supplier;
@@ -195,7 +198,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       }
 
       DigestWriter digestWriter =
-          new DigestWriter(directories, repositoryName, rule, starlarkSemantics);
+          DigestWriter.create(env, directories, repositoryName, rule, starlarkSemantics);
+      if (digestWriter == null) {
+        return null;
+      }
 
       boolean excludeRepoFromVendoring = true;
       if (VENDOR_DIRECTORY.get(env).isPresent()) { // If vendor mode is on
@@ -221,9 +227,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                 || vendorFile.pinnedRepos().contains(repositoryName);
       }
 
-      String predeclaredInputHash =
-          DigestWriter.computePredeclaredInputHash(rule, starlarkSemantics);
-
       if (shouldUseCachedRepos(env, handler, rule)) {
         // Make sure marker file is up-to-date; correctly describes the current repository state
         var repoState = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env);
@@ -238,7 +241,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         // Then check if the global repo contents cache has this.
         if (repoContentsCache.isEnabled()) {
           for (CandidateRepo candidate :
-              repoContentsCache.getCandidateRepos(predeclaredInputHash)) {
+              repoContentsCache.getCandidateRepos(digestWriter.predeclaredInputHash)) {
             repoState =
                 digestWriter.areRepositoryAndMarkerFileConsistent(
                     handler, env, candidate.recordedInputsFile());
@@ -281,7 +284,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           try {
             cachedRepoDir =
                 repoContentsCache.moveToCache(
-                    repoRoot, digestWriter.markerPath, predeclaredInputHash);
+                    repoRoot, digestWriter.markerPath, digestWriter.predeclaredInputHash);
           } catch (IOException e) {
             throw new RepositoryFunctionException(
                 new IOException(
@@ -679,23 +682,39 @@ private static class DigestWriter {
         ImmutableMap.of(NeverUpToDateRepoRecordedInput.PARSE_FAILURE, "");
 
     private final BlazeDirectories directories;
-    private final Path markerPath;
-    private final String ruleKey;
+    final String predeclaredInputHash;
+    final Path markerPath;
 
-    DigestWriter(
+    private DigestWriter(
         BlazeDirectories directories,
         RepositoryName repositoryName,
-        Rule rule,
-        StarlarkSemantics starlarkSemantics) {
+        String predeclaredInputHash) {
       this.directories = directories;
-      ruleKey = computePredeclaredInputHash(rule, starlarkSemantics);
-      markerPath = getMarkerPath(directories, repositoryName);
+      this.predeclaredInputHash = predeclaredInputHash;
+      this.markerPath = getMarkerPath(directories, repositoryName);
+    }
+
+    /** Returns null if and only if a Skyframe restart is needed. */
+    @Nullable
+    static DigestWriter create(
+        Environment env,
+        BlazeDirectories directories,
+        RepositoryName repositoryName,
+        Rule rule,
+        StarlarkSemantics starlarkSemantics)
+        throws InterruptedException {
+      String predeclaredInputHash =
+          computePredeclaredInputHash(env, rule, starlarkSemantics);
+      if (predeclaredInputHash == null) {
+        return null;
+      }
+      return new DigestWriter(directories, repositoryName, predeclaredInputHash);
     }
 
     void writeMarkerFile(Map<? extends RepoRecordedInput, String> recordedInputValues)
         throws RepositoryFunctionException {
       StringBuilder builder = new StringBuilder();
-      builder.append(ruleKey).append("\n");
+      builder.append(predeclaredInputHash).append("\n");
       for (Map.Entry<RepoRecordedInput, String> recordedInput :
           new TreeMap<RepoRecordedInput, String>(recordedInputValues).entrySet()) {
         String key = recordedInput.getKey().toString();
@@ -743,7 +762,7 @@ RepoDirectoryState areRepositoryAndMarkerFileConsistent(
       try {
         String content = FileSystemUtils.readContent(markerPath, ISO_8859_1);
         Map<RepoRecordedInput, String> recordedInputValues =
-            readMarkerFile(content, Preconditions.checkNotNull(ruleKey));
+            readMarkerFile(content, Preconditions.checkNotNull(predeclaredInputHash));
         Optional<String> outdatedReason =
             handler.isAnyRecordedInputOutdated(directories, recordedInputValues, env);
         if (env.valuesMissing()) {
@@ -798,12 +817,28 @@ private static Map<RepoRecordedInput, String> readMarkerFile(
       return Preconditions.checkNotNull(recordedInputValues);
     }
 
-    static String computePredeclaredInputHash(Rule rule, StarlarkSemantics starlarkSemantics) {
-      return new Fingerprint()
-          .addBytes(RuleFormatter.serializeRule(rule).build().toByteArray())
-          .addInt(MARKER_FILE_VERSION)
-          .addBytes(BuildLanguageOptions.stableFingerprint(starlarkSemantics))
-          .hexDigestAndReset();
+    @Nullable
+    @SuppressWarnings("unchecked")
+    static String computePredeclaredInputHash(
+        Environment env, Rule rule, StarlarkSemantics starlarkSemantics)
+        throws InterruptedException {
+      Iterable<String> environAttr = (Iterable<String>) rule.getAttr("$environ");
+      ImmutableSet<String> environKeys =
+          environAttr != null ? ImmutableSet.copyOf(environAttr) : ImmutableSet.of();
+      var unsortedEnviron = RepositoryFunction.getEnvVarValues(env, environKeys);
+      if (unsortedEnviron == null) {
+        return null;
+      }
+      var environ = RepoRecordedInput.EnvVar.wrap(unsortedEnviron);
+      var fp =
+          new Fingerprint()
+              .addBytes(RuleFormatter.serializeRule(rule).build().toByteArray())
+              .addInt(MARKER_FILE_VERSION)
+              .addBytes(BuildLanguageOptions.stableFingerprint(starlarkSemantics))
+              .addInt(environ.size());
+      environ.forEach(
+          (key, value) -> fp.addString(key.toString()).addNullableString(value.orElse(null)));
+      return fp.hexDigestAndReset();
     }
 
     private static Path getMarkerPath(BlazeDirectories directories, RepositoryName repo) {

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java

@@ -20,6 +20,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -266,8 +267,12 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env)
    * registering them as dependencies.
    */
   @Nullable
-  public static ImmutableMap<String, Optional<String>> getEnvVarValues(
+  public static ImmutableSortedMap<String, Optional<String>> getEnvVarValues(
       Environment env, Set<String> keys) throws InterruptedException {
+    // Avoid a dependency on PrecomputedValue.REPO_ENV in this case.
+    if (keys.isEmpty()) {
+      return ImmutableSortedMap.of();
+    }
     Map<String, Optional<String>> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
     if (environ == null) {
       return null;
@@ -286,7 +291,7 @@ public static ImmutableMap<String, Optional<String>> getEnvVarValues(
         repoEnv.put(key, Optional.of(value));
       }
     }
-    return repoEnv.buildKeepingLast();
+    return ImmutableSortedMap.copyOf(repoEnv.buildKeepingLast());
   }
 
   /**