Skip to content

Commit

Permalink
Do not store the repository name in RepoSpec
Browse files Browse the repository at this point in the history
The name can instead be set centrally in `BzlmodRepoRuleFunction`. This removes some unnecessary glue code and paves the way for rewriting canonical repo names based on knowledge of the full dep graph.

Work towards bazelbuild#20997

Closes bazelbuild#21026.

PiperOrigin-RevId: 604256186
Change-Id: Ia49123f016d1512074b04ec458f017f4bbb88233
  • Loading branch information
fmeum authored and iancha1992 committed Feb 5, 2024
1 parent 0cb96ee commit 4ca917b
Show file tree
Hide file tree
Showing 20 changed files with 193 additions and 321 deletions.
8 changes: 4 additions & 4 deletions MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,10 @@ public void workspaceInit(
"local_config_platform",
new NonRegistryOverride() {
@Override
public RepoSpec getRepoSpec(RepositoryName repoName) {
public RepoSpec getRepoSpec() {
return RepoSpec.builder()
.setRuleClassName("local_config_platform")
.setAttributes(
AttributeValues.create(ImmutableMap.of("name", repoName.getName())))
.setAttributes(AttributeValues.create(ImmutableMap.of()))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
import com.google.devtools.build.lib.cmdline.RepositoryName;

/** Specifies that a module should be retrieved from an archive. */
@AutoValue
Expand Down Expand Up @@ -55,9 +54,8 @@ public static ArchiveOverride create(

/** Returns the {@link RepoSpec} that defines this repository. */
@Override
public RepoSpec getRepoSpec(RepositoryName repoName) {
public RepoSpec getRepoSpec() {
return new ArchiveRepoSpecBuilder()
.setRepoName(repoName.getName())
.setUrls(getUrls())
.setIntegrity(getIntegrity())
.setStripPrefix(getStripPrefix())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,6 @@ public ArchiveRepoSpecBuilder() {
attrBuilder = new ImmutableMap.Builder<>();
}

@CanIgnoreReturnValue
public ArchiveRepoSpecBuilder setRepoName(String repoName) {
attrBuilder.put("name", repoName);
return this;
}

@CanIgnoreReturnValue
public ArchiveRepoSpecBuilder setUrls(ImmutableList<String> urls) {
attrBuilder.put("urls", urls);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
import com.google.devtools.build.lib.cmdline.RepositoryName;

/** Specifies that a module should be retrieved from a Git repository. */
@AutoValue
Expand Down Expand Up @@ -54,17 +53,15 @@ public static GitOverride create(

/** Returns the {@link RepoSpec} that defines this repository. */
@Override
public RepoSpec getRepoSpec(RepositoryName repoName) {
GitRepoSpecBuilder builder = new GitRepoSpecBuilder();
builder
.setRepoName(repoName.getName())
public RepoSpec getRepoSpec() {
return new GitRepoSpecBuilder()
.setRemote(getRemote())
.setCommit(getCommit())
.setPatches(getPatches())
.setPatchCmds(getPatchCmds())
.setPatchArgs(ImmutableList.of("-p" + getPatchStrip()))
.setInitSubmodules(getInitSubmodules());
return builder.build();
.setInitSubmodules(getInitSubmodules())
.build();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@ public GitRepoSpecBuilder() {
attrBuilder = new ImmutableMap.Builder<>();
}

@CanIgnoreReturnValue
public GitRepoSpecBuilder setRepoName(String repoName) {
return setAttr("name", repoName);
}

@CanIgnoreReturnValue
public GitRepoSpecBuilder setRemote(String remoteRepoUrl) {
return setAttr("remote", remoteRepoUrl);
Expand Down Expand Up @@ -80,11 +75,6 @@ public GitRepoSpecBuilder setPatches(List<String> patches) {
return setAttr("patches", patches);
}

@CanIgnoreReturnValue
public GitRepoSpecBuilder setPatchTool(String patchTool) {
return setAttr("patch_tool", patchTool);
}

@CanIgnoreReturnValue
public GitRepoSpecBuilder setPatchArgs(List<String> patchArgs) {
return setAttr("patch_args", patchArgs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
Expand Down Expand Up @@ -187,8 +186,7 @@ private <T> T parseJson(String jsonString, String url, Class<T> klass) throws IO
}

@Override
public RepoSpec getRepoSpec(
ModuleKey key, RepositoryName repoName, ExtendedEventHandler eventHandler)
public RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
throws IOException, InterruptedException {
String jsonUrl =
constructUrl(
Expand All @@ -208,22 +206,19 @@ public RepoSpec getRepoSpec(
{
ArchiveSourceJson typedSourceJson =
parseJson(jsonString.get(), jsonUrl, ArchiveSourceJson.class);
return createArchiveRepoSpec(
typedSourceJson, getBazelRegistryJson(eventHandler), key, repoName);
return createArchiveRepoSpec(typedSourceJson, getBazelRegistryJson(eventHandler), key);
}
case "local_path":
{
LocalPathSourceJson typedSourceJson =
parseJson(jsonString.get(), jsonUrl, LocalPathSourceJson.class);
return createLocalPathRepoSpec(
typedSourceJson, getBazelRegistryJson(eventHandler), key, repoName);
return createLocalPathRepoSpec(typedSourceJson, getBazelRegistryJson(eventHandler), key);
}
case "git_repository":
{
GitRepoSourceJson typedSourceJson =
parseJson(jsonString.get(), jsonUrl, GitRepoSourceJson.class);
return createGitRepoSpec(
typedSourceJson, getBazelRegistryJson(eventHandler), key, repoName);
return createGitRepoSpec(typedSourceJson);
}
default:
throw new IOException(
Expand All @@ -249,10 +244,7 @@ private Optional<BazelRegistryJson> getBazelRegistryJson(ExtendedEventHandler ev
}

private RepoSpec createLocalPathRepoSpec(
LocalPathSourceJson sourceJson,
Optional<BazelRegistryJson> bazelRegistryJson,
ModuleKey key,
RepositoryName repoName)
LocalPathSourceJson sourceJson, Optional<BazelRegistryJson> bazelRegistryJson, ModuleKey key)
throws IOException {
String path = sourceJson.path;
if (!PathFragment.isAbsolute(path)) {
Expand All @@ -278,17 +270,12 @@ private RepoSpec createLocalPathRepoSpec(
return RepoSpec.builder()
.setRuleClassName("local_repository")
.setAttributes(
AttributeValues.create(
ImmutableMap.of(
"name", repoName.getName(), "path", PathFragment.create(path).toString())))
AttributeValues.create(ImmutableMap.of("path", PathFragment.create(path).toString())))
.build();
}

private RepoSpec createArchiveRepoSpec(
ArchiveSourceJson sourceJson,
Optional<BazelRegistryJson> bazelRegistryJson,
ModuleKey key,
RepositoryName repoName)
ArchiveSourceJson sourceJson, Optional<BazelRegistryJson> bazelRegistryJson, ModuleKey key)
throws IOException {
URL sourceUrl = sourceJson.url;
if (sourceUrl == null) {
Expand Down Expand Up @@ -332,7 +319,6 @@ private RepoSpec createArchiveRepoSpec(
}

return new ArchiveRepoSpecBuilder()
.setRepoName(repoName.getName())
.setUrls(urls.build())
.setIntegrity(sourceJson.integrity)
.setStripPrefix(Strings.nullToEmpty(sourceJson.stripPrefix))
Expand All @@ -342,14 +328,8 @@ private RepoSpec createArchiveRepoSpec(
.build();
}

private RepoSpec createGitRepoSpec(
GitRepoSourceJson sourceJson,
Optional<BazelRegistryJson> bazelRegistryJson,
ModuleKey key,
RepositoryName repoName)
throws IOException {
private RepoSpec createGitRepoSpec(GitRepoSourceJson sourceJson) {
return new GitRepoSpecBuilder()
.setRepoName(repoName.getName())
.setRemote(sourceJson.remote)
.setCommit(sourceJson.commit)
.setShallowSince(sourceJson.shallowSince)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
import com.google.devtools.build.lib.cmdline.RepositoryName;

/** Specifies that a module should be retrieved from a local directory. */
@AutoValue
Expand All @@ -32,11 +31,10 @@ public static LocalPathOverride create(String path) {

/** Returns the {@link RepoSpec} that defines this repository. */
@Override
public RepoSpec getRepoSpec(RepositoryName repoName) {
public RepoSpec getRepoSpec() {
return RepoSpec.builder()
.setRuleClassName("local_repository")
.setAttributes(
AttributeValues.create(ImmutableMap.of("name", repoName.getName(), "path", getPath())))
.setAttributes(AttributeValues.create(ImmutableMap.of("path", getPath())))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
ruleClass,
Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));

Map<String, Object> attributes = Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k));
Map<String, Object> attributes =
Maps.filterKeys(
Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k)), k -> !k.equals("name"));
String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
RepoSpec repoSpec =
RepoSpec.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public abstract class ModuleKey {
* normal format seen in {@link #getCanonicalRepoName()}) due to backwards compatibility reasons.
* For example, bazel_tools must be known as "@bazel_tools" for WORKSPACE repos to work correctly.
*/
// Keep in sync with src/tools/bzlmod/utils.bzl.
private static final ImmutableMap<String, RepositoryName> WELL_KNOWN_MODULES =
ImmutableMap.of(
"bazel_tools",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
import com.google.devtools.build.lib.cmdline.RepositoryName;

/**
* An override specifying that the module should not be retrieved from a registry or participate in
Expand All @@ -27,7 +26,7 @@
public interface NonRegistryOverride extends ModuleOverride {

/** Returns the {@link RepoSpec} that defines this repository. */
RepoSpec getRepoSpec(RepositoryName repoName);
RepoSpec getRepoSpec();

/**
* Return the exact {@link
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import java.io.IOException;
import java.util.Optional;
Expand All @@ -36,9 +35,9 @@ Optional<ModuleFile> getModuleFile(ModuleKey key, ExtendedEventHandler eventHand

/**
* Retrieves the {@link RepoSpec} object that indicates how the contents of the module identified
* by {@code key} should be materialized as a repo (with name {@code repoName}).
* by {@code key} should be materialized as a repo.
*/
RepoSpec getRepoSpec(ModuleKey key, RepositoryName repoName, ExtendedEventHandler eventHandler)
RepoSpec getRepoSpec(ModuleKey key, ExtendedEventHandler eventHandler)
throws IOException, InterruptedException;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.auto.value.AutoValue;
import com.google.common.collect.Maps;
import com.google.devtools.build.skyframe.SkyValue;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import javax.annotation.Nullable;
Expand All @@ -35,6 +36,11 @@ public abstract class RepoSpec implements SkyValue {

public abstract String ruleClassName();

/**
* All attribute values provided to the repository rule, except for the <code>name</code>
* attribute, which is set in {@link
* com.google.devtools.build.lib.skyframe.BzlmodRepoRuleFunction}.
*/
public abstract AttributeValues attributes();

public static Builder builder() {
Expand All @@ -50,7 +56,22 @@ public abstract static class Builder {

public abstract Builder setAttributes(AttributeValues attributes);

public abstract RepoSpec build();
abstract AttributeValues attributes();

abstract RepoSpec autoBuild();

public final RepoSpec build() {
// Ensure backwards compatibility with old lockfiles that still specify the 'name' attribute.
// TODO: Remove this after both the lockfile version has been bumped and Bazel is built with
// with Bazel 7.1.0.
AttributeValues attributes = attributes();
if (attributes.attributes().containsKey("name")) {
setAttributes(
AttributeValues.create(
Maps.filterKeys(attributes.attributes(), k -> !k.equals("name"))));
}
return autoBuild();
}
}

public boolean isNativeRepoRule() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
.profile(ProfilerTask.BZLMOD, () -> "compute repo spec: " + key.getModuleKey())) {
return registryFactory
.getRegistryWithUrl(key.getRegistryUrl())
.getRepoSpec(
key.getModuleKey(), key.getModuleKey().getCanonicalRepoName(), env.getListener());
.getRepoSpec(key.getModuleKey(), env.getListener());
} catch (IOException e) {
throw new RepoSpecException(
ExternalDepsException.withCauseAndMessage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,9 @@ public RunModuleExtensionResult run(
.setRuleClassName(repoRule.getRuleClass().getName())
.setAttributes(
AttributeValues.create(
Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k))))
Maps.filterKeys(
Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
k -> !k.equals("name"))))
.build();
generatedRepoSpecs.put(name, repoSpec);
}
Expand Down
Loading

0 comments on commit 4ca917b

Please sign in to comment.