Skip to content

Commit

Permalink
Do not fetch yanked versions for selected modules
Browse files Browse the repository at this point in the history
  • Loading branch information
fmeum committed May 7, 2024
1 parent f2f5933 commit a077a02
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ private static Result discoverAndSelect(
var yankedVersionsKeys =
resolvedDepGraph.values().stream()
.filter(m -> m.getRegistry() != null)
.map(m -> YankedVersionsValue.Key.create(m.getName(), m.getRegistry().getUrl()))
.map(m -> YankedVersionsValue.Key.create(m.getKey(), m.getRegistry().getUrl()))
.collect(toImmutableSet());
SkyframeLookupResult yankedVersionsResult = env.getValuesAndExceptions(yankedVersionsKeys);
if (env.valuesMissing()) {
Expand Down Expand Up @@ -319,7 +319,7 @@ private static void checkNoYankedVersions(
YankedVersionsUtil.getYankedInfo(
m.getKey(),
yankedVersionValues.get(
YankedVersionsValue.Key.create(m.getName(), m.getRegistry().getUrl())),
YankedVersionsValue.Key.create(m.getKey(), m.getRegistry().getUrl())),
allowedYankedVersions);
if (yankedInfo.isPresent()) {
throw new BazelModuleResolutionFunctionException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.function.Predicate;

/**
* Represents a Bazel module registry that serves a list of module metadata from a static HTTP
Expand Down Expand Up @@ -254,13 +255,7 @@ private <T> T parseJson(String jsonString, String url, Class<T> klass) throws IO
@Override
public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
throws IOException, InterruptedException {
String jsonUrl =
constructUrl(
uri.toString(),
"modules",
key.getName(),
key.getVersion().toString(),
SOURCE_JSON_FILENAME);
String jsonUrl = getSourceJsonUrl(key);
Optional<String> jsonString = grabJsonFile(jsonUrl, eventHandler, /* useChecksum= */ true);
if (jsonString.isEmpty()) {
throw new FileNotFoundException(
Expand Down Expand Up @@ -292,7 +287,11 @@ public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
}
}

@SuppressWarnings("OptionalAssignedToNull")
private String getSourceJsonUrl(ModuleKey key) {
return constructUrl(
getUrl(), "modules", key.getName(), key.getVersion().toString(), SOURCE_JSON_FILENAME);
}

private Optional<BazelRegistryJson> getBazelRegistryJson(ExtendedEventHandler eventHandler)
throws IOException, InterruptedException {
if (bazelRegistryJson == null) {
Expand Down Expand Up @@ -440,6 +439,24 @@ public Optional<ImmutableMap<Version, String>> getYankedVersions(
}
}

@Override
public boolean shouldFetchYankedVersions(
ModuleKey selectedModuleKey, Predicate<String> fileHashIsKnown) {
// If the source.json hash is known, this module has been selected before when selection
// succeeded, which means that
// either:
// * it wasn't yanked at that point in time and any successful selection since then has not seen
// a higher module
// version, or
// * it was yanked at that point in time, but explicitly allowed via
// BZLMOD_ALLOW_YANKED_VERSIONS.
// In both cases, we don't fetch yanked versions.
// TODO: Should we store whether we are in the second case and refetch yanked versions if the
// environment variable
// changes?
return !fileHashIsKnown.test(getSourceJsonUrl(selectedModuleKey));
}

/** Represents fields available in {@code metadata.json} for each module. */
static class MetadataJson {
// There are other attributes in the metadata.json file, but for now, we only care about
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.util.Optional;
import java.util.function.Predicate;

/** A database where module metadata is stored. */
public interface Registry extends SkyValue {
Expand Down Expand Up @@ -48,4 +49,11 @@ RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
Optional<ImmutableMap<Version, String>> getYankedVersions(
String moduleName, ExtendedEventHandler eventHandler)
throws IOException, InterruptedException;

/**
* Returns whether the (mutable) yanked versions should be fetched from the registry for the given
* module contained in the post-selection dependency graph, given the knowledge of certain
* registry file hashes contained in the lockfile.
*/
boolean shouldFetchYankedVersions(ModuleKey selectedModuleKey, Predicate<String> fileHashIsKnown);
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,27 @@ public class YankedVersionsFunction implements SkyFunction {
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
var key = (YankedVersionsValue.Key) skyKey.argument();

BazelLockFileValue lockfile = (BazelLockFileValue) env.getValue(BazelLockFileValue.KEY);
if (lockfile == null) {
return null;
}
Registry registry = (Registry) env.getValue(RegistryKey.create(key.getRegistryUrl()));
if (registry == null) {
return null;
}

if (!registry.shouldFetchYankedVersions(
key.getModuleKey(), lockfile.getRegistryFileHashes()::containsKey)) {
return YankedVersionsValue.create(Optional.empty());
}

try (SilentCloseable c =
Profiler.instance()
.profile(
ProfilerTask.BZLMOD, () -> "getting yanked versions: " + key.getModuleName())) {
ProfilerTask.BZLMOD,
() -> "getting yanked versions: " + key.getModuleKey().getName())) {
return YankedVersionsValue.create(
registry.getYankedVersions(key.getModuleName(), env.getListener()));
registry.getYankedVersions(key.getModuleKey().getName(), env.getListener()));
} catch (IOException e) {
env.getListener()
.handle(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ public static YankedVersionsValue create(Optional<ImmutableMap<Version, String>>
abstract static class Key implements SkyKey {
private static final SkyKeyInterner<Key> interner = SkyKey.newInterner();

abstract String getModuleName();
abstract ModuleKey getModuleKey();

abstract String getRegistryUrl();

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

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Predicate;

/**
* Fake implementation of {@link Registry}, where modules can be freely added and stored in memory.
Expand Down Expand Up @@ -99,6 +100,15 @@ public Optional<ImmutableMap<Version, String>> getYankedVersions(
return Optional.ofNullable(yankedVersionMap.get(moduleName));
}

@Override
public boolean shouldFetchYankedVersions(
ModuleKey selectedModuleKey, Predicate<String> fileHashIsKnown) {
return !fileHashIsKnown.test(
"%s/modules/%s/%s/source.json"
.formatted(
url, selectedModuleKey.getName(), selectedModuleKey.getVersion().toString()));
}

@Override
public boolean equals(Object other) {
return other instanceof FakeRegistry fakeRegistry
Expand Down

0 comments on commit a077a02

Please sign in to comment.