Skip to content

Commit

Permalink
Prevent most side effects of yanked modules (#18908)
Browse files Browse the repository at this point in the history 
Yanked module versions no longer contribute dependency requirements or emit `DEBUG` messages for `print()` statements.

Since the module files of yanked modules are still evaluated to learn their compatibility levels, they can still fail to execute.

Closes #18698.

PiperOrigin-RevId: 544059396
Change-Id: I8a37d5c7975947cd717f6e56d97cce467f22178e

Co-authored-by: Fabian Meumertzheim <[email protected]>
  • Loading branch information
@Wyverald @fmeum
Wyverald and fmeum authored Jul 11, 2023
1 parent ab35c6f commit da329e3
Show file tree
Hide file tree
Showing 25 changed files with 363 additions and 183 deletions.
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import com.google.devtools.build.lib.bazel.bzlmod.RepoSpec;
import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionEvalFunction;
import com.google.devtools.build.lib.bazel.bzlmod.SingleExtensionUsagesFunction;
import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil;
import com.google.devtools.build.lib.bazel.commands.FetchCommand;
import com.google.devtools.build.lib.bazel.commands.ModqueryCommand;
import com.google.devtools.build.lib.bazel.commands.SyncCommand;
Expand Down Expand Up @@ -570,7 +571,7 @@ public ImmutableList<Injected> getPrecomputedValues() {
BazelModuleResolutionFunction.BAZEL_COMPATIBILITY_MODE, bazelCompatibilityMode),
PrecomputedValue.injected(BazelLockFileFunction.LOCKFILE_MODE, bazelLockfileMode),
PrecomputedValue.injected(
BazelModuleResolutionFunction.ALLOWED_YANKED_VERSIONS, allowedYankedVersions));
YankedVersionsUtil.ALLOWED_YANKED_VERSIONS, allowedYankedVersions));
}

@Override
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ java_library(
"SingleExtensionUsagesFunction.java",
"StarlarkBazelModule.java",
"TypeCheckedTag.java",
"YankedVersionsUtil.java",
],
deps = [
":common",
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup
(ClientEnvironmentValue)
env.getValue(
ClientEnvironmentFunction.key(
BazelModuleResolutionFunction.BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
YankedVersionsUtil.BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
if (allowedYankedVersionsFromEnv == null) {
return null;
}
Expand All @@ -185,7 +185,7 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup
toImmutableMap(e -> e.getKey(), e -> ((LocalPathOverride) e.getValue()).getPath()));

ImmutableList<String> yankedVersions =
ImmutableList.copyOf(BazelModuleResolutionFunction.ALLOWED_YANKED_VERSIONS.get(env));
ImmutableList.copyOf(YankedVersionsUtil.ALLOWED_YANKED_VERSIONS.get(env));
Boolean ignoreDevDeps = ModuleFileFunction.IGNORE_DEV_DEPS.get(env);
String compatabilityMode =
BazelModuleResolutionFunction.BAZEL_COMPATIBILITY_MODE.get(env).name();
Expand Down
147 changes: 4 additions & 143 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,38 +15,30 @@

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

import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableCollection;
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.analysis.BlazeVersionInfo;
import com.google.devtools.build.lib.bazel.BazelVersion;
import com.google.devtools.build.lib.bazel.bzlmod.InterimModule.DepSpec;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
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;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import javax.annotation.Nullable;

/**
Expand All @@ -59,22 +51,11 @@ public class BazelModuleResolutionFunction implements SkyFunction {
new Precomputed<>("check_direct_dependency");
public static final Precomputed<BazelCompatibilityMode> BAZEL_COMPATIBILITY_MODE =
new Precomputed<>("bazel_compatibility_mode");
public static final Precomputed<List<String>> ALLOWED_YANKED_VERSIONS =
new Precomputed<>("allowed_yanked_versions");

public static final String BZLMOD_ALLOWED_YANKED_VERSIONS_ENV = "BZLMOD_ALLOW_YANKED_VERSIONS";

@Override
@Nullable
public SkyValue compute(SkyKey skyKey, Environment env)
throws SkyFunctionException, InterruptedException {

ClientEnvironmentValue allowedYankedVersionsFromEnv =
(ClientEnvironmentValue)
env.getValue(ClientEnvironmentFunction.key(BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
if (allowedYankedVersionsFromEnv == null) {
return null;
}
RootModuleFileValue root =
(RootModuleFileValue) env.getValue(ModuleFileValue.KEY_FOR_ROOT_MODULE);
if (root == null) {
Expand Down Expand Up @@ -104,12 +85,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
Objects.requireNonNull(BAZEL_COMPATIBILITY_MODE.get(env)),
env.getListener());

verifyYankedVersions(
resolvedDepGraph,
parseYankedVersions(
allowedYankedVersionsFromEnv.getValue(),
Objects.requireNonNull(ALLOWED_YANKED_VERSIONS.get(env))),
env.getListener());
checkNoYankedVersions(resolvedDepGraph);

ImmutableMap<ModuleKey, Module> finalDepGraph =
computeFinalDepGraph(resolvedDepGraph, root.getOverrides(), env.getListener());
Expand Down Expand Up @@ -194,125 +170,10 @@ public static void checkBazelCompatibility(
}
}

/**
* Parse a set of allowed yanked version from command line flag (--allowed_yanked_versions) and
* environment variable (ALLOWED_YANKED_VERSIONS). If `all` is specified, return Optional.empty();
* otherwise returns the set of parsed modulel key.
*/
private Optional<ImmutableSet<ModuleKey>> parseYankedVersions(
String allowedYankedVersionsFromEnv, List<String> allowedYankedVersionsFromFlag)
throws BazelModuleResolutionFunctionException {
ImmutableSet.Builder<ModuleKey> allowedYankedVersionBuilder = new ImmutableSet.Builder<>();
if (allowedYankedVersionsFromEnv != null) {
if (parseModuleKeysFromString(
allowedYankedVersionsFromEnv,
allowedYankedVersionBuilder,
String.format(
"envirnoment variable %s=%s",
BZLMOD_ALLOWED_YANKED_VERSIONS_ENV, allowedYankedVersionsFromEnv))) {
return Optional.empty();
}
}
for (String allowedYankedVersions : allowedYankedVersionsFromFlag) {
if (parseModuleKeysFromString(
allowedYankedVersions,
allowedYankedVersionBuilder,
String.format("command line flag --allow_yanked_versions=%s", allowedYankedVersions))) {
return Optional.empty();
}
}
return Optional.of(allowedYankedVersionBuilder.build());
}

/**
* Parse of a comma-separated list of module version(s) of the form '<module name>@<version>' or
* 'all' from the string. Returns true if 'all' is present, otherwise returns false.
*/
private boolean parseModuleKeysFromString(
String input, ImmutableSet.Builder<ModuleKey> allowedYankedVersionBuilder, String context)
private static void checkNoYankedVersions(ImmutableMap<ModuleKey, InterimModule> depGraph)
throws BazelModuleResolutionFunctionException {
ImmutableList<String> moduleStrs = ImmutableList.copyOf(Splitter.on(',').split(input));

for (String moduleStr : moduleStrs) {
if (moduleStr.equals("all")) {
return true;
}

if (moduleStr.isEmpty()) {
continue;
}

String[] pieces = moduleStr.split("@", 2);

if (pieces.length != 2) {
throw new BazelModuleResolutionFunctionException(
ExternalDepsException.withMessage(
Code.VERSION_RESOLUTION_ERROR,
"Parsing %s failed, module versions must be of the form '<module name>@<version>'",
context),
Transience.PERSISTENT);
}

if (!RepositoryName.VALID_MODULE_NAME.matcher(pieces[0]).matches()) {
throw new BazelModuleResolutionFunctionException(
ExternalDepsException.withMessage(
Code.VERSION_RESOLUTION_ERROR,
"Parsing %s failed, invalid module name '%s': valid names must 1) only contain"
+ " lowercase letters (a-z), digits (0-9), dots (.), hyphens (-), and"
+ " underscores (_); 2) begin with a lowercase letter; 3) end with a lowercase"
+ " letter or digit.",
context,
pieces[0]),
Transience.PERSISTENT);
}

Version version;
try {
version = Version.parse(pieces[1]);
} catch (ParseException e) {
throw new BazelModuleResolutionFunctionException(
ExternalDepsException.withCauseAndMessage(
Code.VERSION_RESOLUTION_ERROR,
e,
"Parsing %s failed, invalid version specified for module: %s",
context,
pieces[1]),
Transience.PERSISTENT);
}

allowedYankedVersionBuilder.add(ModuleKey.create(pieces[0], version));
}
return false;
}

private static void verifyYankedVersions(
ImmutableMap<ModuleKey, InterimModule> depGraph,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions,
ExtendedEventHandler eventHandler)
throws BazelModuleResolutionFunctionException, InterruptedException {
// Check whether all resolved modules are either not yanked or allowed. Modules with a
// NonRegistryOverride are ignored as their metadata is not available whatsoever.
for (InterimModule m : depGraph.values()) {
if (m.getKey().equals(ModuleKey.ROOT) || m.getRegistry() == null) {
continue;
}
Optional<ImmutableMap<Version, String>> yankedVersions;
try {
yankedVersions = m.getRegistry().getYankedVersions(m.getKey().getName(), eventHandler);
} catch (IOException e) {
eventHandler.handle(
Event.warn(
String.format(
"Could not read metadata file for module %s: %s", m.getKey(), e.getMessage())));
continue;
}
if (yankedVersions.isEmpty()) {
continue;
}
String yankedInfo = yankedVersions.get().get(m.getVersion());
if (yankedInfo != null
&& allowedYankedVersions.isPresent()
&& !allowedYankedVersions.get().contains(m.getKey())) {
if (m.getYankedInfo().isPresent()) {
throw new BazelModuleResolutionFunctionException(
ExternalDepsException.withMessage(
Code.VERSION_RESOLUTION_ERROR,
Expand All @@ -322,7 +183,7 @@ private static void verifyYankedVersions(
+ "continue using this version, allow it using the --allow_yanked_versions "
+ "flag or the BZLMOD_ALLOW_YANKED_VERSIONS env variable.",
m.getKey(),
yankedInfo),
m.getYankedInfo().get()),
Transience.PERSISTENT);
}
}
Expand Down
9 changes: 8 additions & 1 deletion 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,6 +50,9 @@ 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 @@ -102,7 +105,8 @@ public static Builder builder() {
.setName("")
.setVersion(Version.EMPTY)
.setKey(ModuleKey.ROOT)
.setCompatibilityLevel(0);
.setCompatibilityLevel(0)
.setYankedInfo(Optional.empty());
}

/**
Expand Down Expand Up @@ -133,6 +137,9 @@ 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
Loading

0 comments on commit da329e3

Please sign in to comment.