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.

Sign up for GitHub

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

Jump to bottom

Prevent most side effects of yanked modules #18698

Closed
wants to merge 2 commits into from
Closed

Prevent most side effects of yanked modules #18698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e98fbc
Prevent most side effects of yanked modules
fmeum Jun 20, 2023
338a97d
Address review comments
fmeum Jun 25, 2023
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
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
View file
Open in desktop
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 @@ -590,7 +591,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,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 @@ -180,7 +180,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
137 changes: 7 additions & 130 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,17 @@

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;
Expand All @@ -43,10 +39,8 @@
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,10 +53,6 @@ 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
Expand All @@ -71,7 +61,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)

ClientEnvironmentValue allowedYankedVersionsFromEnv =
(ClientEnvironmentValue)
env.getValue(ClientEnvironmentFunction.key(BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
env.getValue(
fmeum marked this conversation as resolved.
Show resolved Hide resolved
ClientEnvironmentFunction.key(
YankedVersionsUtil.BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
if (allowedYankedVersionsFromEnv == null) {
return null;
}
Expand Down Expand Up @@ -104,12 +96,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());
verifyYankedVersions(resolvedDepGraph);

ImmutableMap<ModuleKey, Module> finalDepGraph =
computeFinalDepGraph(resolvedDepGraph, root.getOverrides(), env.getListener());
Expand Down Expand Up @@ -194,125 +181,15 @@ 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)
private static void verifyYankedVersions(ImmutableMap<ModuleKey, InterimModule> depGraph)
fmeum marked this conversation as resolved.
Show resolved Hide resolved
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)
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) {
fmeum marked this conversation as resolved.
Show resolved Hide resolved
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 +199,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
Open in desktop
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