diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java index b5beb7880a0ed8..d489a008588783 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DelegateTypeAdapterFactory.java +++ b/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, 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 index 669fda492d471d..f509b01a194cfc 100644 --- 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 @@ -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) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java index a955eb70c81bfd..913d3d24afe199 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 84a12f17566ce5..8718920f4b14ab 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/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 getRecordedFileInputs(); + public abstract ImmutableSortedMap getRecordedFileInputs(); - public abstract ImmutableMap getRecordedDirentsInputs(); + public abstract ImmutableSortedMap getRecordedDirentsInputs(); - public abstract ImmutableMap> getEnvVariables(); + public abstract ImmutableSortedMap> getEnvVariables(); public abstract ImmutableMap getGeneratedRepoSpecs(); @@ -72,13 +73,13 @@ public abstract static class Builder { public abstract Builder setUsagesDigest(byte[] digest); public abstract Builder setRecordedFileInputs( - ImmutableMap value); + ImmutableSortedMap value); public abstract Builder setRecordedDirentsInputs( - ImmutableMap value); + ImmutableSortedMap value); public abstract Builder setEnvVariables( - ImmutableMap> value); + ImmutableSortedMap> value); public abstract Builder setGeneratedRepoSpecs(ImmutableMap value); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java index bb5bfb83760438..9065194928ef2c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java +++ b/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 recordedFileInputs, - ImmutableMap recordedDirentsInputs, - ImmutableMap> recordedEnvVarInputs, + ImmutableSortedMap recordedFileInputs, + ImmutableSortedMap recordedDirentsInputs, + ImmutableSortedMap> recordedEnvVarInputs, ImmutableMap generatedRepoSpecs, ModuleExtensionMetadata moduleExtensionMetadata, ImmutableTable recordedRepoMappingEntries) {} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 47cdb8f228c06a..91a98bc5e33891 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/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 getRecordedFileInputs() { + public ImmutableSortedMap getRecordedFileInputs() { return ImmutableSortedMap.copyOf(recordedFileInputs); } - public ImmutableMap getRecordedDirentsInputs() { + public ImmutableSortedMap getRecordedDirentsInputs() { return ImmutableSortedMap.copyOf(recordedDirentsInputs); } - public ImmutableMap> getRecordedEnvVarInputs() + public ImmutableSortedMap> 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) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 8b987a9f825132..59b79202b21653 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/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 getRecordedDirTreeInputs() { - return ImmutableMap.copyOf(recordedDirTreeInputs); + public ImmutableSortedMap getRecordedDirTreeInputs() { + return ImmutableSortedMap.copyOf(recordedDirTreeInputs); } @StarlarkMethod( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 87f887adb07dd8..f1edc2b714d89e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 0819cd21c51648..bc6c39f4855a30 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/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> wrap( + public static ImmutableSortedMap> wrap( Map> 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) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index a738356830acba..82e6a822a5f05b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/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 recordedInputValues) throws RepositoryFunctionException { StringBuilder builder = new StringBuilder(); - builder.append(ruleKey).append("\n"); + builder.append(predeclaredInputHash).append("\n"); for (Map.Entry recordedInput : new TreeMap(recordedInputValues).entrySet()) { String key = recordedInput.getKey().toString(); @@ -743,7 +762,7 @@ RepoDirectoryState areRepositoryAndMarkerFileConsistent( try { String content = FileSystemUtils.readContent(markerPath, ISO_8859_1); Map recordedInputValues = - readMarkerFile(content, Preconditions.checkNotNull(ruleKey)); + readMarkerFile(content, Preconditions.checkNotNull(predeclaredInputHash)); Optional outdatedReason = handler.isAnyRecordedInputOutdated(directories, recordedInputValues, env); if (env.valuesMissing()) { @@ -798,12 +817,28 @@ private static Map 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 environAttr = (Iterable) rule.getAttr("$environ"); + ImmutableSet 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) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 62ea1b970e7b4b..13b997951b7444 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/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> getEnvVarValues( + public static ImmutableSortedMap> getEnvVarValues( Environment env, Set keys) throws InterruptedException { + // Avoid a dependency on PrecomputedValue.REPO_ENV in this case. + if (keys.isEmpty()) { + return ImmutableSortedMap.of(); + } Map> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys); if (environ == null) { return null; @@ -286,7 +291,7 @@ public static ImmutableMap> getEnvVarValues( repoEnv.put(key, Optional.of(value)); } } - return repoEnv.buildKeepingLast(); + return ImmutableSortedMap.copyOf(repoEnv.buildKeepingLast()); } /** diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java index 2f409e94e40538..7ecbd82d0606e0 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModuleTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.lib.cmdline.Label; import java.util.Optional; import net.starlark.java.eval.Dict; @@ -45,18 +46,18 @@ public void setUp() throws Exception { LockFileModuleExtension.builder() .setBzlTransitiveDigest(new byte[] {1, 2, 3}) .setUsagesDigest(new byte[] {4, 5, 6}) - .setRecordedFileInputs(ImmutableMap.of()) - .setRecordedDirentsInputs(ImmutableMap.of()) - .setEnvVariables(ImmutableMap.of()) + .setRecordedFileInputs(ImmutableSortedMap.of()) + .setRecordedDirentsInputs(ImmutableSortedMap.of()) + .setEnvVariables(ImmutableSortedMap.of()) .setGeneratedRepoSpecs(ImmutableMap.of()) .build(); reproducibleResult = LockFileModuleExtension.builder() .setBzlTransitiveDigest(new byte[] {1, 2, 3}) .setUsagesDigest(new byte[] {4, 5, 6}) - .setRecordedFileInputs(ImmutableMap.of()) - .setRecordedDirentsInputs(ImmutableMap.of()) - .setEnvVariables(ImmutableMap.of()) + .setRecordedFileInputs(ImmutableSortedMap.of()) + .setRecordedDirentsInputs(ImmutableSortedMap.of()) + .setEnvVariables(ImmutableSortedMap.of()) .setGeneratedRepoSpecs(ImmutableMap.of()) .setModuleExtensionMetadata( LockfileModuleExtensionMetadata.of(