Skip to content

Commit

Permalink
Extract yanked versions into a SkyFunction
Browse files Browse the repository at this point in the history
Ensures that yanked version information is only downloaded once per module name, which reduces the number of files fetched during module resolution. On my machine, this reduces `bazel mod deps` runtime on Bazel itself by ~15%.

This also prepares for storing yanked version information in the lockfile.

Work towards bazelbuild#20369

Closes bazelbuild#21909.

PiperOrigin-RevId: 625617556
Change-Id: Ie4184def3b868470f93d584bbbbd7f704e1e9a82
  • Loading branch information
fmeum authored and Kila2 committed May 13, 2024
1 parent 4764397 commit 8a3d6ff
Show file tree
Hide file tree
Showing 16 changed files with 200 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction;
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.YankedVersionsFunction;
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.ModCommand;
Expand Down Expand Up @@ -271,6 +272,7 @@ SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace
.addSkyFunction(SkyFunctions.SINGLE_EXTENSION_EVAL, singleExtensionEvalFunction)
.addSkyFunction(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction())
.addSkyFunction(SkyFunctions.REPO_SPEC, new RepoSpecFunction(registryFactory))
.addSkyFunction(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction(registryFactory))
.addSkyFunction(
SkyFunctions.MODULE_EXTENSION_REPO_MAPPING_ENTRIES,
new ModuleExtensionRepoMappingEntriesFunction());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ java_library(
"SingleExtensionEvalValue.java",
"SingleExtensionUsagesValue.java",
"SingleVersionOverride.java",
"YankedVersionsValue.java",
],
deps = [
":common",
Expand Down Expand Up @@ -196,6 +197,7 @@ java_library(
"SingleExtensionUsagesFunction.java",
"StarlarkBazelModule.java",
"TypeCheckedTag.java",
"YankedVersionsFunction.java",
"YankedVersionsUtil.java",
],
deps = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ public ModuleFileFunction(
this.builtinModules = builtinModules;
}

private static class State implements Environment.SkyKeyComputeState {
GetModuleFileResult getModuleFileResult;
}

@Nullable
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
Expand Down Expand Up @@ -150,28 +154,47 @@ public SkyValue compute(SkyKey skyKey, Environment env)

ModuleFileValue.Key moduleFileKey = (ModuleFileValue.Key) skyKey;
ModuleKey moduleKey = moduleFileKey.getModuleKey();
GetModuleFileResult getModuleFileResult;
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) {
getModuleFileResult =
getModuleFile(moduleKey, moduleFileKey.getOverride(), allowedYankedVersions, env);
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;
}
}
if (getModuleFileResult == null) {
return null;
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();
}
String moduleFileHash =
new Fingerprint().addBytes(getModuleFileResult.moduleFile.getContent()).hexDigestAndReset();
new Fingerprint()
.addBytes(state.getModuleFileResult.moduleFile.getContent())
.hexDigestAndReset();

ModuleThreadContext moduleThreadContext =
execModuleFile(
getModuleFileResult.moduleFile,
getModuleFileResult.registry,
state.getModuleFileResult.moduleFile,
state.getModuleFileResult.registry,
moduleKey,
// 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= */ getModuleFileResult.yankedInfo != null,
/* printIsNoop= */ yankedInfo.isPresent(),
starlarkSemantics,
starlarkEnv,
env.getListener(),
Expand Down Expand Up @@ -200,7 +223,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
module.getVersion());
}

if (getModuleFileResult.yankedInfo != null) {
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
Expand All @@ -212,7 +235,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
.setVersion(module.getVersion())
.setCompatibilityLevel(module.getCompatibilityLevel())
.setRegistry(module.getRegistry())
.setYankedInfo(Optional.of(getModuleFileResult.yankedInfo))
.setYankedInfo(yankedInfo)
.build(),
moduleFileHash);
}
Expand Down Expand Up @@ -392,19 +415,13 @@ private static ModuleThreadContext execModuleFile(

private static class GetModuleFileResult {
ModuleFile moduleFile;
// `yankedInfo` is non-null if and only if the module has been yanked and hasn't been
// allowlisted.
@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,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions,
Environment env)
ModuleKey key, @Nullable ModuleOverride override, 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 @@ -481,10 +498,6 @@ private GetModuleFileResult getModuleFile(
}
result.moduleFile = moduleFile.get();
result.registry = registry;
result.yankedInfo =
YankedVersionsUtil.getYankedInfo(
registry, key, allowedYankedVersions, env.getListener())
.orElse(null);
return result;
} catch (IOException e) {
throw errorf(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright 2021 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

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

import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.net.URISyntaxException;
import java.util.Optional;
import javax.annotation.Nullable;

/**
* A simple SkyFunction that computes a {@link RepoSpec} for the given {@link InterimModule} by
* fetching required information from its {@link Registry}.
*/
public class YankedVersionsFunction implements SkyFunction {
private final RegistryFactory registryFactory;

public YankedVersionsFunction(RegistryFactory registryFactory) {
this.registryFactory = registryFactory;
}

@Override
@Nullable
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
var key = (YankedVersionsValue.Key) skyKey.argument();
try (SilentCloseable c =
Profiler.instance()
.profile(
ProfilerTask.BZLMOD, () -> "getting yanked versions: " + key.getModuleName())) {
return YankedVersionsValue.create(
registryFactory
.getRegistryWithUrl(key.getRegistryUrl())
.getYankedVersions(key.getModuleName(), env.getListener()));
} catch (IOException e) {
env.getListener()
.handle(
Event.warn(
String.format(
"Could not read metadata file for module %s: %s", key, e.getMessage())));
// This is failing open: If we can't read the metadata file, we allow yanked modules to be
// fetched.
return YankedVersionsValue.create(Optional.empty());
} catch (URISyntaxException e) {
// This should never happen since we obtain the registry URL from an already constructed
// registry.
throw new IllegalStateException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,10 @@

import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import java.io.IOException;
import java.util.List;
import java.util.Optional;

Expand Down Expand Up @@ -63,39 +59,23 @@ static Optional<ImmutableSet<ModuleKey>> parseAllowedYankedVersions(
return Optional.of(allowedYankedVersionBuilder.build());
}

/**
* Returns the reason for the given module being yanked, or {@code Optional.empty()} if the module
* is not yanked or explicitly allowed despite being yanked.
*/
static Optional<String> getYankedInfo(
Registry registry,
ModuleKey key,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions,
ExtendedEventHandler eventHandler)
throws InterruptedException {
Optional<ImmutableMap<Version, String>> yankedVersions;
try {
yankedVersions = registry.getYankedVersions(key.getName(), eventHandler);
} catch (IOException e) {
eventHandler.handle(
Event.warn(
String.format(
"Could not read metadata file for module %s: %s", key, e.getMessage())));
// This is failing open: If we can't read the metadata file, we allow yanked modules to be
// fetched.
return Optional.empty();
}
if (yankedVersions.isEmpty()) {
return Optional.empty();
}
String yankedInfo = yankedVersions.get().get(key.getVersion());
if (yankedInfo != null
&& allowedYankedVersions.isPresent()
&& !allowedYankedVersions.get().contains(key)) {
return Optional.of(yankedInfo);
} else {
return Optional.empty();
}
YankedVersionsValue yankedVersionsValue,
Optional<ImmutableSet<ModuleKey>> allowedYankedVersions) {
return yankedVersionsValue
.yankedVersions()
.flatMap(
yankedVersions -> {
String yankedInfo = yankedVersions.get(key.getVersion());
if (yankedInfo != null
&& allowedYankedVersions.isPresent()
&& !allowedYankedVersions.get().contains(key)) {
return Optional.of(yankedInfo);
} else {
return Optional.empty();
}
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright 2021 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

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

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.skyframe.SkyFunctions;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Optional;

/** A class holding information about the versions of a particular module that have been yanked. */
@AutoValue
public abstract class YankedVersionsValue implements SkyValue {

public abstract Optional<ImmutableMap<Version, String>> yankedVersions();

public static YankedVersionsValue create(Optional<ImmutableMap<Version, String>> yankedVersions) {
return new AutoValue_YankedVersionsValue(yankedVersions);
}

/** The key for {@link YankedVersionsFunction}. */
@AutoCodec
@AutoValue
abstract static class Key implements SkyKey {
private static final SkyKeyInterner<Key> interner = SkyKey.newInterner();

abstract String getModuleName();

abstract String getRegistryUrl();

@AutoCodec.Instantiator
static Key create(String moduleName, String registryUrl) {
return interner.intern(new AutoValue_YankedVersionsValue_Key(moduleName, registryUrl));
}

@Override
public SkyFunctionName functionName() {
return SkyFunctions.YANKED_VERSIONS;
}

@Override
public SkyKeyInterner<Key> getSkyKeyInterner() {
return interner;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ public final class SkyFunctions {
public static final SkyFunctionName BAZEL_FETCH_ALL =
SkyFunctionName.createHermetic("BAZEL_FETCH_ALL");
public static final SkyFunctionName REPO_SPEC = SkyFunctionName.createNonHermetic("REPO_SPEC");
public static final SkyFunctionName YANKED_VERSIONS =
SkyFunctionName.createNonHermetic("YANKED_VERSIONS");

public static final SkyFunctionName MODULE_EXTENSION_REPO_MAPPING_ENTRIES =
SkyFunctionName.createHermetic("MODULE_EXTENSION_REPO_MAPPING_ENTRIES");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction;
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.YankedVersionsFunction;
import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.BazelCompatibilityMode;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode;
Expand Down Expand Up @@ -180,6 +181,7 @@ public ImmutableMap<SkyFunctionName, SkyFunction> getSkyFunctions(BlazeDirectori
new SingleExtensionEvalFunction(directories, ImmutableMap::of, downloadManager))
.put(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction())
.put(SkyFunctions.REPO_SPEC, new RepoSpecFunction(FakeRegistry.DEFAULT_FACTORY))
.put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction(FakeRegistry.DEFAULT_FACTORY))
.put(
SkyFunctions.MODULE_EXTENSION_REPO_MAPPING_ENTRIES,
new ModuleExtensionRepoMappingEntriesFunction())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public void setup() throws Exception {
.put(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction())
.put(SkyFunctions.BAZEL_MODULE_RESOLUTION, resolutionFunctionMock)
.put(SkyFunctions.REPO_SPEC, new RepoSpecFunction(registryFactory))
.put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction(registryFactory))
.put(
SkyFunctions.MODULE_EXTENSION_REPO_MAPPING_ENTRIES,
new ModuleExtensionRepoMappingEntriesFunction())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ public void setup() throws Exception {
ImmutableMap.of()))
.put(SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(rootDirectory))
.put(SkyFunctions.REPO_SPEC, new RepoSpecFunction(registryFactory))
.put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction(registryFactory))
.put(
SkyFunctions.MODULE_EXTENSION_REPO_MAPPING_ENTRIES,
new ModuleExtensionRepoMappingEntriesFunction())
Expand Down
Loading

0 comments on commit 8a3d6ff

Please sign in to comment.