Skip to content

Commit

Permalink
Revert special handling of yanked module files
Browse files Browse the repository at this point in the history 
This effectively reverts most of 2a2a474: Module files of yanked versions are evaluated just like those of any other versions and "yankedness" is only checked for the final dep graph after selection.

This greatly simplifies incremental fetching of (inherently mutable) yanked version information with the new lockfile format.

Work towards #20369

RELNOTES: `print` statements in module files are now only executed for the root module and modules subject to non-registry overrides (e.g. `local_path_override`).

Closes #22083.

PiperOrigin-RevId: 627953972
Change-Id: Ie0aba02d187e000450a89ad2cd281c173582880a
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Apr 25, 2024
1 parent 1ff310b commit 45982b4
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 139 deletions.
68 changes: 62 additions & 6 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@

package com.google.devtools.build.lib.bazel.bzlmod;

import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil.BZLMOD_ALLOWED_YANKED_VERSIONS_ENV;

import com.google.common.base.Joiner;
import com.google.common.base.Strings;
Expand All @@ -35,6 +37,8 @@
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
import com.google.devtools.build.lib.skyframe.ClientEnvironmentFunction;
import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
Expand All @@ -44,6 +48,7 @@
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import javax.annotation.Nullable;

Expand All @@ -62,6 +67,23 @@ public class BazelModuleResolutionFunction implements SkyFunction {
@Nullable
public SkyValue compute(SkyKey skyKey, Environment env)
throws BazelModuleResolutionFunctionException, InterruptedException {
ClientEnvironmentValue allowedYankedVersionsFromEnv =
(ClientEnvironmentValue)
env.getValue(ClientEnvironmentFunction.key(BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
if (allowedYankedVersionsFromEnv == null) {
return null;
}

Optional<ImmutableSet<ModuleKey>> allowedYankedVersions;
try {
allowedYankedVersions =
YankedVersionsUtil.parseAllowedYankedVersions(
allowedYankedVersionsFromEnv.getValue(),
Objects.requireNonNull(YankedVersionsUtil.ALLOWED_YANKED_VERSIONS.get(env)));
} catch (ExternalDepsException e) {
throw new BazelModuleResolutionFunctionException(e, Transience.PERSISTENT);
}

RootModuleFileValue root =
(RootModuleFileValue) env.getValue(ModuleFileValue.KEY_FOR_ROOT_MODULE);
if (root == null) {
Expand All @@ -70,7 +92,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)

var state = env.getState(ModuleResolutionComputeState::new);
if (state.selectionResult == null) {
state.selectionResult = discoverAndSelect(env, root);
state.selectionResult = discoverAndSelect(env, root, allowedYankedVersions);
if (state.selectionResult == null) {
return null;
}
Expand Down Expand Up @@ -108,7 +130,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

@Nullable
private static Selection.Result discoverAndSelect(Environment env, RootModuleFileValue root)
private static Selection.Result discoverAndSelect(
Environment env,
RootModuleFileValue root,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions)
throws BazelModuleResolutionFunctionException, InterruptedException {
ImmutableMap<ModuleKey, InterimModule> initialDepGraph;
try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, "discovery")) {
Expand All @@ -130,6 +155,24 @@ private static Selection.Result discoverAndSelect(Environment env, RootModuleFil
}
ImmutableMap<ModuleKey, InterimModule> resolvedDepGraph = selectionResult.getResolvedDepGraph();

var yankedVersionsKeys =
resolvedDepGraph.values().stream()
.filter(m -> m.getRegistry() != null)
.map(m -> YankedVersionsValue.Key.create(m.getName(), m.getRegistry().getUrl()))
.collect(toImmutableSet());
SkyframeLookupResult yankedVersionsResult = env.getValuesAndExceptions(yankedVersionsKeys);
if (env.valuesMissing()) {
return null;
}
var yankedVersionValues =
yankedVersionsKeys.stream()
.collect(
toImmutableMap(
key -> key, key -> (YankedVersionsValue) yankedVersionsResult.get(key)));
if (yankedVersionValues.values().stream().anyMatch(Objects::isNull)) {
return null;
}

try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.BZLMOD, "verify root module direct deps")) {
verifyRootModuleDirectDepsAreAccurate(
Expand All @@ -149,7 +192,7 @@ private static Selection.Result discoverAndSelect(Environment env, RootModuleFil

try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.BZLMOD, "check no yanked versions")) {
checkNoYankedVersions(resolvedDepGraph);
checkNoYankedVersions(resolvedDepGraph, yankedVersionValues, allowedYankedVersions);
}

return selectionResult;
Expand Down Expand Up @@ -246,10 +289,23 @@ public static void checkBazelCompatibility(
}
}

private static void checkNoYankedVersions(ImmutableMap<ModuleKey, InterimModule> depGraph)
private static void checkNoYankedVersions(
ImmutableMap<ModuleKey, InterimModule> depGraph,
ImmutableMap<YankedVersionsValue.Key, YankedVersionsValue> yankedVersionValues,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions)
throws BazelModuleResolutionFunctionException {
for (InterimModule m : depGraph.values()) {
if (m.getYankedInfo().isPresent()) {
if (m.getRegistry() == null) {
// Non-registry modules do not have yanked versions.
continue;
}
Optional<String> yankedInfo =
YankedVersionsUtil.getYankedInfo(
m.getKey(),
yankedVersionValues.get(
YankedVersionsValue.Key.create(m.getName(), m.getRegistry().getUrl())),
allowedYankedVersions);
if (yankedInfo.isPresent()) {
throw new BazelModuleResolutionFunctionException(
ExternalDepsException.withMessage(
Code.VERSION_RESOLUTION_ERROR,
Expand All @@ -259,7 +315,7 @@ private static void checkNoYankedVersions(ImmutableMap<ModuleKey, InterimModule>
+ "continue using this version, allow it using the --allow_yanked_versions "
+ "flag or the BZLMOD_ALLOW_YANKED_VERSIONS env variable.",
m.getKey(),
m.getYankedInfo().get()),
yankedInfo.get()),
Transience.PERSISTENT);
}
}
Expand Down
9 changes: 1 addition & 8 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ public abstract class InterimModule extends ModuleBase {
/** List of bazel compatible versions that would run/fail this module */
public abstract ImmutableList<String> getBazelCompatibility();

/** The reason why this module was yanked or empty if it hasn't been yanked. */
public abstract Optional<String> getYankedInfo();

/** The specification of a dependency. */
@AutoValue
public abstract static class DepSpec {
Expand Down Expand Up @@ -105,8 +102,7 @@ public static Builder builder() {
.setName("")
.setVersion(Version.EMPTY)
.setKey(ModuleKey.ROOT)
.setCompatibilityLevel(0)
.setYankedInfo(Optional.empty());
.setCompatibilityLevel(0);
}

/**
Expand Down Expand Up @@ -137,9 +133,6 @@ public abstract static class Builder {
/** Optional; defaults to {@link #setName}. */
public abstract Builder setRepoName(String value);

/** Optional; defaults to {@link Optional#empty()}. */
public abstract Builder setYankedInfo(Optional<String> value);

public abstract Builder setBazelCompatibility(ImmutableList<String> value);

abstract ImmutableList.Builder<String> bazelCompatibilityBuilder();
Expand Down
75 changes: 13 additions & 62 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

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.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.bazel.bzlmod.CompiledModuleFile.IncludeStatement;
Expand Down Expand Up @@ -117,10 +116,6 @@ public ModuleFileFunction(
}

private static class State implements Environment.SkyKeyComputeState {
// The following field is used during non-root module file evaluation. We store the module file
// here so that a later attempt to retrieve yanked versions wouldn't be overly expensive.
GetModuleFileResult getModuleFileResult;

// The following fields are used during root module file evaluation. We try to compile the root
// module file itself first, and then read, parse, and compile any included module files layer
// by layer, in a BFS fashion (hence the `horizon` field). Finally, everything is collected into
Expand Down Expand Up @@ -152,54 +147,24 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return null;
}

Optional<ImmutableSet<ModuleKey>> allowedYankedVersions;
try {
allowedYankedVersions =
YankedVersionsUtil.parseAllowedYankedVersions(
allowedYankedVersionsFromEnv.getValue(),
Objects.requireNonNull(YankedVersionsUtil.ALLOWED_YANKED_VERSIONS.get(env)));
} catch (ExternalDepsException e) {
throw new ModuleFileFunctionException(e, SkyFunctionException.Transience.PERSISTENT);
}

ModuleFileValue.Key moduleFileKey = (ModuleFileValue.Key) skyKey;
ModuleKey moduleKey = moduleFileKey.getModuleKey();
State state = env.getState(State::new);
if (state.getModuleFileResult == null) {
try (SilentCloseable c =
Profiler.instance()
.profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) {
state.getModuleFileResult = getModuleFile(moduleKey, moduleFileKey.getOverride(), env);
}
if (state.getModuleFileResult == null) {
return null;
}
GetModuleFileResult getModuleFileResult;
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) {
getModuleFileResult = getModuleFile(moduleKey, moduleFileKey.getOverride(), env);
}
Optional<String> yankedInfo;
if (state.getModuleFileResult.registry != null) {
YankedVersionsValue yankedVersionsValue =
(YankedVersionsValue)
env.getValue(
YankedVersionsValue.Key.create(
moduleKey.getName(), state.getModuleFileResult.registry.getUrl()));
if (yankedVersionsValue == null) {
return null;
}
yankedInfo =
YankedVersionsUtil.getYankedInfo(moduleKey, yankedVersionsValue, allowedYankedVersions);
} else {
yankedInfo = Optional.empty();
if (getModuleFileResult == null) {
return null;
}
String moduleFileHash =
new Fingerprint()
.addBytes(state.getModuleFileResult.moduleFile.getContent())
.hexDigestAndReset();
new Fingerprint().addBytes(getModuleFileResult.moduleFile.getContent()).hexDigestAndReset();

CompiledModuleFile compiledModuleFile;
try {
compiledModuleFile =
CompiledModuleFile.parseAndCompile(
state.getModuleFileResult.moduleFile,
getModuleFileResult.moduleFile,
moduleKey,
starlarkSemantics,
starlarkEnv,
Expand All @@ -221,16 +186,18 @@ public SkyValue compute(SkyKey skyKey, Environment env)
// Dev dependencies should always be ignored if the current module isn't the root module
/* ignoreDevDeps= */ true,
builtinModules,
// We try to prevent most side effects of yanked modules, in particular print().
/* printIsNoop= */ yankedInfo.isPresent(),
// Disable printing for modules from registries. We don't want them to be able to spam
// the console during resolution, but module files potentially edited by the user as
// part of a non-registry override should permit printing to aid debugging.
/* printIsNoop= */ getModuleFileResult.registry != null,
starlarkSemantics,
env.getListener(),
SymbolGenerator.create(skyKey));

// Perform some sanity checks.
InterimModule module;
try {
module = moduleThreadContext.buildModule(state.getModuleFileResult.registry);
module = moduleThreadContext.buildModule(getModuleFileResult.registry);
} catch (EvalException e) {
env.getListener().handle(Event.error(e.getMessageWithStack()));
throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey);
Expand All @@ -250,22 +217,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
module.getVersion());
}

if (yankedInfo.isPresent()) {
// Yanked modules should not have observable side effects such as adding dependency
// requirements, so we drop those from the constructed module. We do have to preserve the
// compatibility level as it influences the set of versions the yanked version can be updated
// to during selection.
return NonRootModuleFileValue.create(
InterimModule.builder()
.setKey(module.getKey())
.setName(module.getName())
.setVersion(module.getVersion())
.setCompatibilityLevel(module.getCompatibilityLevel())
.setRegistry(module.getRegistry())
.setYankedInfo(yankedInfo)
.build(),
moduleFileHash);
}

return NonRootModuleFileValue.create(module, moduleFileHash);
}
Expand Down
Loading

0 comments on commit 45982b4

Please sign in to comment.