Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.2.0] Detect extension staleness due root module name/version change #22282

Merged
merged 1 commit into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)) {
Expand All @@ -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 -> {
Expand All @@ -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
Expand All @@ -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<>();

Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
.create();
}

public static Gson createModuleExtensionUsagesHashGson() {
public static Gson createSingleExtensionUsagesValueHashGson() {
return newGsonBuilder().create();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,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())
Expand Down Expand Up @@ -280,9 +280,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");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,26 @@
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.autocodec.AutoCodec;
import com.google.devtools.build.skyframe.AbstractSkyKey;
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
Expand All @@ -54,6 +64,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);
}
Expand Down
Loading
Loading