Skip to content

Commit

Permalink
bzlmod: Store canonical repo names in SelectionValue
Browse files Browse the repository at this point in the history 
    (bazelbuild/bazel#13316)

    Up until now we assume the canonical repo name of a module is simply the module name. This doesn't work with multiple-version overrides. This CL addresses this issue; the canonical repo name will be ${moduleName}.${moduleVersion} for any module.

    This does mean that we need to put in some extra work to build reverse lookup maps, and especially add extra logic in RepositoryMappingFunction to make sure that WORKSPACE references to repos generated by modules can still work (i.e. if B is a module, something that points to "@b" gets rewritten to "@B.1.0", the canonical repo name)

    PiperOrigin-RevId: 385785118
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 0f6a066 commit 5ac09b3
Show file tree
Hide file tree
Showing 13 changed files with 308 additions and 392 deletions.
42 changes: 19 additions & 23 deletions ...el/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleHelperImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import java.io.IOException;
Expand All @@ -30,14 +31,15 @@ public final class BzlmodRepoRuleHelperImpl implements BzlmodRepoRuleHelper {
public Optional<RepoSpec> getRepoSpec(Environment env, String repositoryName)
throws InterruptedException, IOException {

ModuleFileValue root = (ModuleFileValue) env.getValue(ModuleFileValue.keyForRootModule());
RootModuleFileValue root =
(RootModuleFileValue) env.getValue(ModuleFileValue.keyForRootModule());
if (env.valuesMissing()) {
return null;
}
ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();

// Step 1: Look for repositories defined by non-registry overrides.
Optional<RepoSpec> repoSpec = checkRepoFromNonRegistryOverrides(overrides, repositoryName);
Optional<RepoSpec> repoSpec = checkRepoFromNonRegistryOverrides(root, repositoryName);
if (repoSpec.isPresent()) {
return repoSpec;
}
Expand All @@ -61,36 +63,30 @@ public Optional<RepoSpec> getRepoSpec(Environment env, String repositoryName)
}

private static Optional<RepoSpec> checkRepoFromNonRegistryOverrides(
ImmutableMap<String, ModuleOverride> overrides, String repositoryName) {
if (overrides.containsKey(repositoryName)) {
ModuleOverride override = overrides.get(repositoryName);
if (override instanceof NonRegistryOverride) {
return Optional.of(((NonRegistryOverride) override).getRepoSpec(repositoryName));
}
RootModuleFileValue root, String repositoryName) {
String moduleName = root.getNonRegistryOverrideCanonicalRepoNameLookup().get(repositoryName);
if (moduleName == null) {
return Optional.empty();
}
return Optional.empty();
NonRegistryOverride override = (NonRegistryOverride) root.getOverrides().get(moduleName);
return Optional.of(override.getRepoSpec(repositoryName));
}

private static Optional<RepoSpec> checkRepoFromBazelModules(
SelectionValue selectionValue,
ImmutableMap<String, ModuleOverride> overrides,
ExtendedEventHandler eventlistener,
ExtendedEventHandler eventListener,
String repositoryName)
throws InterruptedException, IOException {
for (ModuleKey moduleKey : selectionValue.getDepGraph().keySet()) {
// TODO(pcloudy): Support multiple version override.
// Currently we assume there is only one version for each module, therefore the module name is
// the repository name, but that's not the case if multiple version of the same module are
// allowed.
if (moduleKey.getName().equals(repositoryName)) {
Module module = selectionValue.getDepGraph().get(moduleKey);
Registry registry = checkNotNull(module.getRegistry());
RepoSpec repoSpec = registry.getRepoSpec(moduleKey, repositoryName, eventlistener);
repoSpec = maybeAppendAdditionalPatches(repoSpec, overrides.get(moduleKey.getName()));
return Optional.of(repoSpec);
}
ModuleKey moduleKey = selectionValue.getCanonicalRepoNameLookup().get(repositoryName);
if (moduleKey == null) {
return Optional.empty();
}
return Optional.empty();
Module module = selectionValue.getDepGraph().get(moduleKey);
Registry registry = checkNotNull(module.getRegistry());
RepoSpec repoSpec = registry.getRepoSpec(moduleKey, repositoryName, eventListener);
repoSpec = maybeAppendAdditionalPatches(repoSpec, overrides.get(moduleKey.getName()));
return Optional.of(repoSpec);
}

private static RepoSpec maybeAppendAdditionalPatches(RepoSpec repoSpec, ModuleOverride override) {
Expand Down
45 changes: 27 additions & 18 deletions ...ild.bazel/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@
package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.ArrayDeque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Queue;
import java.util.Set;
import javax.annotation.Nullable;

/**
Expand All @@ -37,55 +40,61 @@ public class DiscoveryFunction implements SkyFunction {
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws SkyFunctionException, InterruptedException {
ModuleFileValue root = (ModuleFileValue) env.getValue(ModuleFileValue.keyForRootModule());
RootModuleFileValue root =
(RootModuleFileValue) env.getValue(ModuleFileValue.keyForRootModule());
if (root == null) {
return null;
}
ModuleKey rootModuleKey = ModuleKey.create(root.getModule().getName(), "");
ModuleKey rootModuleKey = ModuleKey.create(root.getModule().getName(), Version.EMPTY);
ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();
Map<ModuleKey, Module> depGraph = new HashMap<>();
depGraph.put(
rootModuleKey, rewriteDepKeys(root.getModule(), overrides, rootModuleKey.getName()));
Queue<ModuleKey> unexpanded = new ArrayDeque<>();
unexpanded.add(rootModuleKey);
// TODO(wyv): currently we expand the "unexpanded" keys one by one. We should try to expand them
// all at once, using `env.getValues`.
while (!unexpanded.isEmpty()) {
Module module = depGraph.get(unexpanded.remove());
for (ModuleKey depKey : module.getDeps().values()) {
if (depGraph.containsKey(depKey)) {
continue;
Set<SkyKey> unexpandedSkyKeys = new HashSet<>();
while (!unexpanded.isEmpty()) {
Module module = depGraph.get(unexpanded.remove());
for (ModuleKey depKey : module.getDeps().values()) {
if (depGraph.containsKey(depKey)) {
continue;
}
unexpandedSkyKeys.add(ModuleFileValue.key(depKey, overrides.get(depKey.getName())));
}
ModuleFileValue dep =
(ModuleFileValue)
env.getValue(ModuleFileValue.key(depKey, overrides.get(depKey.getName())));
if (dep == null) {
}
Map<SkyKey, SkyValue> result = env.getValues(unexpandedSkyKeys);
for (Map.Entry<SkyKey, SkyValue> entry : result.entrySet()) {
ModuleKey depKey = ((ModuleFileValue.Key) entry.getKey()).getModuleKey();
ModuleFileValue moduleFileValue = (ModuleFileValue) entry.getValue();
if (moduleFileValue == null) {
// Don't return yet. Try to expand any other unexpanded nodes before returning.
depGraph.put(depKey, null);
} else {
depGraph.put(depKey, rewriteDepKeys(dep.getModule(), overrides, rootModuleKey.getName()));
depGraph.put(
depKey,
rewriteDepKeys(moduleFileValue.getModule(), overrides, rootModuleKey.getName()));
unexpanded.add(depKey);
}
}
}
if (env.valuesMissing()) {
return null;
}
return DiscoveryValue.create(
root.getModule().getName(), ImmutableMap.copyOf(depGraph), overrides);
return DiscoveryValue.create(root.getModule().getName(), ImmutableMap.copyOf(depGraph));
}

private static Module rewriteDepKeys(
Module module, ImmutableMap<String, ModuleOverride> overrides, String rootModuleName) {
return module.withDepKeysTransformed(
depKey -> {
String newVersion = depKey.getVersion();
Version newVersion = depKey.getVersion();

@Nullable ModuleOverride override = overrides.get(depKey.getName());
if (override instanceof NonRegistryOverride || rootModuleName.equals(depKey.getName())) {
newVersion = "";
newVersion = Version.EMPTY;
} else if (override instanceof SingleVersionOverride) {
String overrideVersion = ((SingleVersionOverride) override).getVersion();
Version overrideVersion = ((SingleVersionOverride) override).getVersion();
if (!overrideVersion.isEmpty()) {
newVersion = overrideVersion;
}
Expand Down
24 changes: 18 additions & 6 deletions ...ld.bazel/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 @@ -15,9 +15,14 @@

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

import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.NonRootModuleFileValue;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
Expand Down Expand Up @@ -106,7 +111,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throw errorf("The MODULE.bazel file of %s declares overrides", moduleKey);
}

return ModuleFileValue.create(module, ImmutableMap.of());
return NonRootModuleFileValue.create(module);
}

private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Environment env)
Expand All @@ -128,7 +133,16 @@ private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Envir
if (rootOverride != null) {
throw errorf("invalid override for the root module found: %s", rootOverride);
}
return ModuleFileValue.create(module, overrides);
ImmutableMap<String, String> nonRegistryOverrideCanonicalRepoNameLookup =
Maps.filterValues(overrides, override -> override instanceof NonRegistryOverride)
.keySet()
.stream()
.collect(
toImmutableMap(
name -> ModuleKey.create(name, Version.EMPTY).getCanonicalRepoName(),
name -> name));
return RootModuleFileValue.create(
module, overrides, nonRegistryOverrideCanonicalRepoNameLookup);
}

private ModuleFileGlobals execModuleFile(
Expand Down Expand Up @@ -169,14 +183,12 @@ private GetModuleFileResult getModuleFile(
// 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.
if (override instanceof NonRegistryOverride) {
// The canonical repo name of a module with a non-registry override is always the name of the
// module.
String repoName = key.getName();
String canonicalRepoName = key.getCanonicalRepoName();
RepositoryDirectoryValue repoDir =
(RepositoryDirectoryValue)
env.getValue(
RepositoryDirectoryValue.key(
RepositoryName.createFromValidStrippedName(repoName)));
RepositoryName.createFromValidStrippedName(canonicalRepoName)));
if (repoDir == null) {
return null;
}
Expand Down
42 changes: 32 additions & 10 deletions ...build.bazel/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@
import javax.annotation.Nullable;

/** The result of {@link ModuleFileFunction}. */
@AutoValue
public abstract class ModuleFileValue implements SkyValue {

public static final ModuleKey ROOT_MODULE_KEY = ModuleKey.create("", "");
public static final ModuleKey ROOT_MODULE_KEY = ModuleKey.create("", Version.EMPTY);

/**
* The module resulting from the module file evaluation. Note, in particular, that the version of
Expand All @@ -40,15 +39,40 @@ public abstract class ModuleFileValue implements SkyValue {
*/
public abstract Module getModule();

/** The {@link ModuleFileValue} for non-root modules. */
@AutoValue
public abstract static class NonRootModuleFileValue extends ModuleFileValue {

public static NonRootModuleFileValue create(Module module) {
return new AutoValue_ModuleFileValue_NonRootModuleFileValue(module);
}
}

/**
* The overrides specified by the evaluated module file. The key is the module name and the value
* is the override itself.
* The {@link ModuleFileValue} for the root module, containing additional information about
* overrides.
*/
public abstract ImmutableMap<String, ModuleOverride> getOverrides();
@AutoValue
public abstract static class RootModuleFileValue extends ModuleFileValue {
/**
* The overrides specified by the evaluated module file. The key is the module name and the
* value is the override itself.
*/
public abstract ImmutableMap<String, ModuleOverride> getOverrides();

public static ModuleFileValue create(
Module module, ImmutableMap<String, ModuleOverride> overrides) {
return new AutoValue_ModuleFileValue(module, overrides);
/**
* A mapping from a canonical repo name to the name of the module. Only works for modules with
* non-registry overrides.
*/
public abstract ImmutableMap<String, String> getNonRegistryOverrideCanonicalRepoNameLookup();

public static RootModuleFileValue create(
Module module,
ImmutableMap<String, ModuleOverride> overrides,
ImmutableMap<String, String> nonRegistryOverrideCanonicalRepoNameLookup) {
return new AutoValue_ModuleFileValue_RootModuleFileValue(
module, overrides, nonRegistryOverrideCanonicalRepoNameLookup);
}
}

public static Key key(ModuleKey moduleKey, @Nullable ModuleOverride override) {
Expand All @@ -65,7 +89,6 @@ public static Key keyForRootModule() {
}

/** {@link SkyKey} for {@link ModuleFileValue} computation. */
@AutoCodec.VisibleForSerialization
@AutoCodec
@AutoValue
abstract static class Key implements SkyKey {
Expand All @@ -76,7 +99,6 @@ abstract static class Key implements SkyKey {
@Nullable
abstract ModuleOverride getOverride();

@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
static Key create(ModuleKey moduleKey, @Nullable ModuleOverride override) {
return interner.intern(new AutoValue_ModuleFileValue_Key(moduleKey, override));
Expand Down
5 changes: 5 additions & 0 deletions .../bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,9 @@ public final String toString() {
+ "@"
+ (getVersion().isEmpty() ? "_" : getVersion().toString());
}

/** Returns the canonical name of the repo backing this module. */
public String getCanonicalRepoName() {
return getName() + "." + getVersion();
}
}
Loading

0 comments on commit 5ac09b3

Please sign in to comment.