From 7b646656d05233e16ab10398acfd140bac9ec86c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 19 Feb 2024 13:02:35 +0100 Subject: [PATCH] Store usages hash in module extension lockfile entries Work towards #20369 --- MODULE.bazel.lock | 2 +- .../bazel/bzlmod/BazelDepGraphFunction.java | 10 ------- .../lib/bazel/bzlmod/BazelLockFileModule.java | 30 +++++++++++-------- .../lib/bazel/bzlmod/BazelLockFileValue.java | 2 +- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 16 +++++++--- .../bazel/bzlmod/LockFileModuleExtension.java | 5 ++++ .../bazel/bzlmod/ModuleExtensionUsage.java | 20 ++++++------- .../bzlmod/SingleExtensionEvalFunction.java | 26 +++++++--------- src/test/tools/bzlmod/MODULE.bazel.lock | 14 ++++++++- 9 files changed, 70 insertions(+), 55 deletions(-) diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 02832ad230bd9f..4d52af037c2be3 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -2741,7 +2741,7 @@ "general": { "bzlTransitiveDigest": "r8gQnSLwon27gWD77J8mb3DIe4v3gtn7J/rsic53Qyw=", "accumulatedFileDigests": { - "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "90bec989ef7d229ef67c24ad140987a20c64343695a944fc04c7266815f89ade", + "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "296106749fed660caef0a1abafde88e194ee1ab1e93f6e44cb3371ed990cbf22", "@@//:MODULE.bazel": "c440045e7f8f6ff70a870646e1d3502afe2b63b4f51d794d0b6bfa6cc4b3c007" }, "envVariables": {}, 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 b8099615531637..21dce5d1e59c03 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 @@ -211,16 +211,6 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup compatabilityMode); } - /** - * For each extension usage, we resolve (i.e. canonicalize) its bzl file label. Then we can group - * all usages by the label + name (the ModuleExtensionId). - */ - public static ImmutableTable - getExtensionUsagesById(ImmutableMap depGraph) - throws ExternalDepsException { - return getExtensionUsagesById(depGraph, computeCanonicalRepoNameLookup(depGraph).inverse()); - } - private static ImmutableTable getExtensionUsagesById( ImmutableMap depGraph, 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 2d588414dd86dd..5fb5c1f1581b25 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 @@ -19,7 +19,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; import com.google.common.eventbus.Subscribe; import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions; @@ -33,6 +32,7 @@ import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; import java.io.IOException; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; @@ -107,15 +107,19 @@ public void afterCommand() throws AbruptExitException { updatedExtensionMap = new HashMap<>(); // Keep old extensions if they are still valid. - ImmutableTable 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); + ImmutableMap.Builder + factorsToKeepBuilder = new ImmutableMap.Builder<>(); + for (var factorAndExtension : entry.getValue().entrySet()) { + if (shouldKeepExtension( + moduleExtensionId, factorAndExtension.getKey(), factorAndExtension.getValue())) { + factorsToKeepBuilder.put(factorAndExtension.getKey(), factorAndExtension.getValue()); + } + } + var factorsToKeep = factorsToKeepBuilder.build(); + if (!factorsToKeep.isEmpty()) { + updatedExtensionMap.put(moduleExtensionId, factorsToKeep); } } @@ -164,7 +168,7 @@ public void afterCommand() throws AbruptExitException { private boolean shouldKeepExtension( ModuleExtensionId extensionId, ModuleExtensionEvalFactors lockedExtensionKey, - ImmutableTable oldExtensionUsages) { + LockFileModuleExtension lockedExtension) { // If there is a new event for this extension, compare it with the existing ones ModuleExtensionResolutionEvent extEvent = extensionResolutionEventsMap.get(extensionId); @@ -185,9 +189,11 @@ private boolean shouldKeepExtension( // 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 ModuleExtensionUsage.trimForEvaluation( - moduleResolutionEvent.getExtensionUsagesById().row(extensionId)) - .equals(ModuleExtensionUsage.trimForEvaluation(oldExtensionUsages.row(extensionId))); + return Arrays.equals( + ModuleExtensionUsage.hashForEvaluation( + GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(), + moduleResolutionEvent.getExtensionUsagesById().row(extensionId)), + lockedExtension.getUsagesDigest()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 3c85644a59a996..8625e1345cc919 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -37,7 +37,7 @@ @GenerateTypeAdapter public abstract class BazelLockFileValue implements SkyValue, Postable { - public static final int LOCK_FILE_VERSION = 6; + public static final int LOCK_FILE_VERSION = 7; @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE; 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 a2b62f2789ccc4..265139bd860048 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 @@ -476,8 +476,18 @@ public RepoRecordedInput.Dirents read(JsonReader jsonReader) throws IOException }; public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) { - return new GsonBuilder() + return newGsonBuilder() .setPrettyPrinting() + .registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath, workspaceRoot)) + .create(); + } + + public static Gson createModuleExtensionUsagesHashGson() { + return newGsonBuilder().create(); + } + + private static GsonBuilder newGsonBuilder() { + return new GsonBuilder() .disableHtmlEscaping() .enableComplexMapKeySerialization() .registerTypeAdapterFactory(GenerateTypeAdapter.FACTORY) @@ -488,7 +498,6 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) { .registerTypeAdapterFactory(IMMUTABLE_SET) .registerTypeAdapterFactory(OPTIONAL) .registerTypeAdapterFactory(IMMUTABLE_TABLE) - .registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath, workspaceRoot)) .registerTypeAdapter(Label.class, LABEL_TYPE_ADAPTER) .registerTypeAdapter(RepositoryName.class, REPOSITORY_NAME_TYPE_ADAPTER) .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER) @@ -501,8 +510,7 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) { .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER) .registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER) .registerTypeAdapter( - RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER) - .create(); + RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER); } private GsonTypeAdapterUtil() {} 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 64e401ed6f224b..2af40858371f4b 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 @@ -41,6 +41,9 @@ public static Builder builder() { @SuppressWarnings("mutable") public abstract byte[] getBzlTransitiveDigest(); + @SuppressWarnings("mutable") + public abstract byte[] getUsagesDigest(); + public abstract ImmutableMap getRecordedFileInputs(); public abstract ImmutableMap getRecordedDirentsInputs(); @@ -67,6 +70,8 @@ public abstract static class Builder { public abstract Builder setBzlTransitiveDigest(byte[] digest); + public abstract Builder setUsagesDigest(byte[] digest); + public abstract Builder setRecordedFileInputs( ImmutableMap value); 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 1f93ab0e1a6ef1..f3adcf5f744cbf 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 @@ -22,9 +22,10 @@ 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.Map; import java.util.Optional; import net.starlark.java.syntax.Location; @@ -94,16 +95,15 @@ public static Builder builder() { } /** - * Turns the given collection of usages for a particular extension into an object that can be - * compared for equality with another object obtained in this way and compares equal only if the - * two original collections of usages are equivalent for the purpose of evaluating the extension. + * 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 ImmutableList> trimForEvaluation( - ImmutableMap usages) { - // ImmutableMap#equals doesn't compare the order of entries, but it matters for the evaluation - // of the extension. - return ImmutableList.copyOf( - Maps.transformValues(usages, ModuleExtensionUsage::trimForEvaluation).entrySet()); + static byte[] hashForEvaluation(Gson gson, ImmutableMap usages) { + ImmutableMap trimmedUsages = + ImmutableMap.copyOf(Maps.transformValues(usages, ModuleExtensionUsage::trimForEvaluation)); + String usagesJson = gson.toJson(trimmedUsages); + return Hashing.sha256().hashUnencodedChars(usagesJson).asBytes(); } /** 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 040723437a5b2d..726902bd1927ec 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 @@ -163,7 +163,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) try { SingleExtensionEvalValue singleExtensionEvalValue = tryGettingValueFromLockFile( - env, extensionId, extension, usagesValue, lockfile, lockedExtension); + env, extensionId, extension, usagesValue, lockedExtension); if (singleExtensionEvalValue != null) { return singleExtensionEvalValue; } @@ -217,6 +217,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) extension.getEvalFactors(), LockFileModuleExtension.builder() .setBzlTransitiveDigest(extension.getBzlTransitiveDigest()) + .setUsagesDigest( + ModuleExtensionUsage.hashForEvaluation( + GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(), + usagesValue.getExtensionUsages())) .setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs()) .setRecordedDirentsInputs(moduleExtensionResult.getRecordedDirentsInputs()) .setEnvVariables(extension.getEnvVars()) @@ -243,24 +247,11 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( ModuleExtensionId extensionId, RunnableExtension extension, SingleExtensionUsagesValue usagesValue, - BazelLockFileValue lockfile, LockFileModuleExtension lockedExtension) throws SingleExtensionEvalFunctionException, InterruptedException, NeedsSkyframeRestartException { LockfileMode lockfileMode = BazelLockFileFunction.LOCKFILE_MODE.get(env); - - ImmutableMap lockedExtensionUsages; - try { - // TODO(salmasamy) might be nicer to precompute this table when we construct - // BazelLockFileValue, without adding it to the json file - ImmutableTable extensionUsagesById = - BazelDepGraphFunction.getExtensionUsagesById(lockfile.getModuleDepGraph()); - lockedExtensionUsages = extensionUsagesById.row(extensionId); - } catch (ExternalDepsException e) { - throw new SingleExtensionEvalFunctionException(e, Transience.PERSISTENT); - } - DiffRecorder diffRecorder = new DiffRecorder(/* recordMessages= */ lockfileMode.equals(LockfileMode.ERROR)); try { @@ -280,8 +271,11 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( } // Check extension data in lockfile is still valid, disregarding usage information that is not // relevant for the evaluation of the extension. - if (!ModuleExtensionUsage.trimForEvaluation(usagesValue.getExtensionUsages()) - .equals(ModuleExtensionUsage.trimForEvaluation(lockedExtensionUsages))) { + if (!Arrays.equals( + ModuleExtensionUsage.hashForEvaluation( + GsonTypeAdapterUtil.createModuleExtensionUsagesHashGson(), + usagesValue.getExtensionUsages()), + lockedExtension.getUsagesDigest())) { diffRecorder.record("The usages of the extension '" + extensionId + "' have changed"); } if (didRepoMappingsChange(env, lockedExtension.getRecordedRepoMappingEntries())) { diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index be7dcb5ce953f7..a5b57b0a978c6d 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -1,5 +1,5 @@ { - "lockFileVersion": 6, + "lockFileVersion": 7, "moduleFileHash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "flags": { "cmdRegistries": [ @@ -1037,6 +1037,7 @@ "@@apple_support~//crosstool:setup.bzl%apple_cc_configure_extension": { "general": { "bzlTransitiveDigest": "pMLFCYaRPkgXPQ8vtuNkMfiHfPmRBy6QJfnid4sWfv0=", + "usagesDigest": "G+Wh7ELKrSnuypJ3qascrgbwzrS7Psg28ta9k4FxTH0=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1064,6 +1065,7 @@ "@@bazel_tools//tools/android:android_extensions.bzl%remote_android_tools_extensions": { "general": { "bzlTransitiveDigest": "vsrPPBNf8OgywAYLMcIL1oNm2R8WtbCIL9wgQBUecpA=", + "usagesDigest": "LPw+9iUcnRY1k89ZEMEZ/AtOYIK2LgSeKnaiYVCDaag=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1091,6 +1093,7 @@ "@@bazel_tools//tools/cpp:cc_configure.bzl%cc_configure_extension": { "general": { "bzlTransitiveDigest": "XWy8pzw7/6RclAFWd6/VfUdoXn2SdSpmHOrbfEFJ1ao=", + "usagesDigest": "+YUfVsec8fjmT1xohAjYSdZ308PSnMfO5f1mLULO1sE=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1118,6 +1121,7 @@ "@@bazel_tools//tools/osx:xcode_configure.bzl%xcode_configure_extension": { "general": { "bzlTransitiveDigest": "Qh2bWTU6QW6wkrd87qrU4YeY+SG37Nvw3A0PR4Y0L2Y=", + "usagesDigest": "KB6YWX9sttGdrsit5PNIqvCglz3pKdbLHZ3+3LaOEmY=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1137,6 +1141,7 @@ "@@bazel_tools//tools/sh:sh_configure.bzl%sh_configure_extension": { "general": { "bzlTransitiveDigest": "hp4NgmNjEg5+xgvzfh6L83bt9/aiiWETuNpwNuF1MSU=", + "usagesDigest": "HJTM/9PgqPKgJxJkdQZIZ++0UHXdTqGOip8ROTW9lYI=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1153,6 +1158,7 @@ "@@bazel_tools//tools/test:extensions.bzl%remote_coverage_tools_extension": { "general": { "bzlTransitiveDigest": "AL+K5m+GCP3XRzLPqpKAq4GsjIVDXgUveWm8nih4ju0=", + "usagesDigest": "O3U7dkKl24l4dKwsK8Y1uf1SAa/eEwLfw4BRsHGugwc=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1174,6 +1180,7 @@ "@@buildozer~//:buildozer_binary.bzl%buildozer_binary": { "general": { "bzlTransitiveDigest": "EleDU/FQ1+e/RgkW3aIDmdaxZEthvoWQhsqFTxiSgMI=", + "usagesDigest": "4M43O2sMrjql57L5jklxPaVTLbRZANfEXyODcR5XstI=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1199,6 +1206,7 @@ "@@rules_java~//java:extensions.bzl%toolchains": { "general": { "bzlTransitiveDigest": "Vo8tBC4aLZTSWFbox69vCUJ2B0S7GYQDr5txjiyYfgQ=", + "usagesDigest": "WT1Y55K4woLke9GnlcxBEkjLwibgnv9JARzfhU+c1GM=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -1704,6 +1712,7 @@ "@@rules_jvm_external~//:extensions.bzl%maven": { "general": { "bzlTransitiveDigest": "yXprMX4LqzJwuZlbtT9WNeu7p2iEYw7j4R1NP9pc4Ow=", + "usagesDigest": "L+U25+BzBiliO0i3XrvqC5up0f210dPakCjqg5MBrp4=", "recordedFileInputs": { "@@rules_jvm_external~//rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" }, @@ -2727,6 +2736,7 @@ "@@rules_jvm_external~//:non-module-deps.bzl%non_module_deps": { "general": { "bzlTransitiveDigest": "Td87llNSs5GZ/kAxu6pAqfEXBZ3HdkSqRmUzvIfbFWg=", + "usagesDigest": "HWGzpnxaDzDwBkFxYvT/plvwcfNrPZGTg9ZYibwxNGw=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -2754,6 +2764,7 @@ "@@rules_python~//python/extensions:python.bzl%python": { "general": { "bzlTransitiveDigest": "NGtTMUqs2EEJeXu6mXdpmYRrQGZiJV7S3mxeod3Hm7M=", + "usagesDigest": "2dLy/xmv7+TD8WRYYIxtxZMeS4nrJZGIDSMkYRE0elw=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, @@ -2783,6 +2794,7 @@ "@@rules_python~//python/extensions/private:internal_deps.bzl%internal_deps": { "general": { "bzlTransitiveDigest": "5c1tkdV6L67SQOZWc9MUoS5ZnsSxeDKsh9urs01jZSM=", + "usagesDigest": "MZDuELgGnEk5m0vRt+s02bhPv0B21Oi1jm1KuRqZ5FY=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {},