Skip to content

Commit

Permalink
Do not evaluate module files of yanked modules
Browse files Browse the repository at this point in the history 
`MODULE.bazel` files of yanked module versions are now always treated as
empty. Thus, a dependency on a yanked module is now a true no-op.

Previously, they were evaluated and contributed version requirements to
the overall version resolution process. The module files could also
still execute `print()` and `fail()` statements or contain syntax
errors, all of which affected the build in a way that a yanked module
shouldn't be able to.
  • Loading branch information
@fmeum
fmeum committed Jun 16, 2023
1 parent 959a27b commit 008e619
Show file tree
Hide file tree
Showing 27 changed files with 308 additions and 165 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 @@ -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
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
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
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(
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)
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) {
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
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
62 changes: 58 additions & 4 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 @@ -19,6 +19,7 @@

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.ModuleFileValue.NonRootModuleFileValue;
Expand All @@ -28,6 +29,8 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
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;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down Expand Up @@ -102,13 +105,45 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return computeForRootModule(starlarkSemantics, env);
}

ClientEnvironmentValue allowedYankedVersionsFromEnv =
(ClientEnvironmentValue)
env.getValue(
ClientEnvironmentFunction.key(
YankedVersionsUtil.BZLMOD_ALLOWED_YANKED_VERSIONS_ENV));
if (allowedYankedVersionsFromEnv == null) {
return null;
}

Optional<ImmutableSet<ModuleKey>> allowedYankedVersions;
try {
allowedYankedVersions =
YankedVersionsUtil.parseYankedVersions(
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();
GetModuleFileResult getModuleFileResult =
getModuleFile(moduleKey, moduleFileKey.getOverride(), env);
getModuleFile(moduleKey, moduleFileKey.getOverride(), allowedYankedVersions, env);
if (getModuleFileResult == null) {
return null;
}

if (getModuleFileResult.yankedInfo != null) {
return NonRootModuleFileValue.create(
InterimModule.builder()
.setKey(moduleKey)
.setName(moduleKey.getName())
.setVersion(moduleKey.getVersion())
.setYankedInfo(Optional.of(getModuleFileResult.yankedInfo))
.setRegistry(getModuleFileResult.registry)
.build(),
"");
}

String moduleFileHash =
new Fingerprint().addBytes(getModuleFileResult.moduleFile.getContent()).hexDigestAndReset();

Expand Down Expand Up @@ -224,14 +259,19 @@ private ModuleFileGlobals execModuleFile(
}

private static class GetModuleFileResult {
ModuleFile moduleFile;
// Exactly one of `moduleFile` and `yankedInfo` is non-null.
@Nullable ModuleFile moduleFile;
@Nullable String yankedInfo;
// `registry` can be null if this module has a non-registry override.
@Nullable Registry registry;
}

@Nullable
private GetModuleFileResult getModuleFile(
ModuleKey key, @Nullable ModuleOverride override, Environment env)
ModuleKey key,
@Nullable ModuleOverride override,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions,
Environment env)
throws ModuleFileFunctionException, InterruptedException {
// If there is a non-registry override for this module, we need to fetch the corresponding repo
// first and read the module file from there.
Expand Down Expand Up @@ -289,7 +329,17 @@ private GetModuleFileResult getModuleFile(
if (moduleFile.isEmpty()) {
continue;
}
result.moduleFile = moduleFile.get();
Optional<String> yankedInfo =
YankedVersionsUtil.getYankedInfo(
registry, key, allowedYankedVersions, env.getListener());
if (yankedInfo.isPresent()) {
// If the module is yanked, we do not even evaluate its module file. In particular, its
// dependency requirements are ignored and the evaluation can't have side effects (e.g.,
// print(), fail() or syntax errors).
result.yankedInfo = yankedInfo.get();
} else {
result.moduleFile = moduleFile.get();
}
result.registry = registry;
return result;
} catch (IOException e) {
Expand Down Expand Up @@ -334,5 +384,9 @@ static final class ModuleFileFunctionException extends SkyFunctionException {
ModuleFileFunctionException(ExternalDepsException cause) {
super(cause, Transience.TRANSIENT);
}

ModuleFileFunctionException(ExternalDepsException cause, Transience transience) {
super(cause, transience);
}
}
}
Loading

0 comments on commit 008e619

Please sign in to comment.