Skip to content

Commit 7f9de9e

Browse files
Wyveraldcopybara-github
Wyverald
authored and
copybara-github
committed
Fix canonical label literals for well-known modules (and the main repo)
Currently, the canonical label literal syntax is only used when Bzlmod is enabled. This is not changing. However, right now the label "@@//foo:bar" doesn't work even when Bzlmod is enabled. This is because the current implementation doesn't actually treat @@-prefixed labels as "canonical label literals"; instead, it just tries to find a repo whose canonical name is "@" (i.e. with the first '@' removed). Obviously no such repo exists (since the main repo's canonical name is the empty string). This implementation seems strange, but coupled with the hack where we prepend an '@' to the canonical names of Bzlmod-generated repos, this achieves the effect that canonical label literals are only enabled when Bzlmod is enabled. (de37930 is the CL that introduced this implementation.) This CL fixes it so that we do Bzlmod-exclusive canonical label literals in a more principled way. We remove the hack where we prepend an extra '@' to Bzlmod-generated repo names, and instead just skip repo mapping directly during label parsing if we see two '@'s. Additionally, we can now change the behavior of str(Label) to prepend the extra '@' iff Bzlmod is enabled. Everyone is happy :) Work towards #15916 PiperOrigin-RevId: 469949029 Change-Id: Id77dc13752e9d945a99a823e91a2c4d9d8342623
1 parent 30b3656 commit 7f9de9e

21 files changed

+312
-173
lines changed

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,6 @@ static BazelModuleResolutionValue createValue(
164164
for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
165165
String bestName =
166166
id.getBzlFileLabel().getRepository().getName() + "~" + id.getExtensionName();
167-
if (!bestName.startsWith("@")) {
168-
// We have to special-case extensions hosted in well-known modules, because *all* repos
169-
// generated by Bzlmod have to have an '@'-prefixed name, except well-known modules.
170-
bestName = "@" + bestName;
171-
}
172167
if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
173168
continue;
174169
}

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,6 @@ public abstract class ModuleKey {
3030
* normal format seen in {@link #getCanonicalRepoName()}) due to backwards compatibility reasons.
3131
* For example, bazel_tools must be known as "@bazel_tools" for WORKSPACE repos to work correctly.
3232
*
33-
* <p>NOTE(wyv): We don't prepend an '@' to the repo names of well-known modules. This is because
34-
* we still need the repo name to be 'bazel_tools' (not '@bazel_tools') since the command line
35-
* flags still don't go through repo mapping yet, and they're asking for '@bazel_tools//:thing',
36-
* not '@@bazel_tools//:thing'. We can't switch to the latter syntax because it doesn't work if
37-
* Bzlmod is not enabled. On the other hand, this means we cannot write '@@bazel_tools//:thing' to
38-
* bypass repo mapping at all, which can be awkward.
39-
*
4033
* <p>TODO(wyv): After we make all flag values go through repo mapping, we can remove the concept
4134
* of well-known modules altogether.
4235
*/
@@ -76,6 +69,6 @@ public RepositoryName getCanonicalRepoName() {
7669
return RepositoryName.MAIN;
7770
}
7871
return RepositoryName.createUnvalidated(
79-
String.format("@%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
72+
String.format("%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
8073
}
8174
}

‎src/main/java/com/google/devtools/build/lib/cmdline/Label.java

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,10 @@ private static RepositoryName computeRepoNameWithRepoContext(
144144
? RepositoryName.MAIN
145145
: repoContext.currentRepo();
146146
}
147+
if (parts.repoIsCanonical) {
148+
// This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar").
149+
return RepositoryName.createUnvalidated(parts.repo);
150+
}
147151
return repoContext.repoMapping().get(parts.repo);
148152
}
149153

@@ -621,11 +625,24 @@ public void repr(Printer printer) {
621625

622626
@Override
623627
public void str(Printer printer, StarlarkSemantics semantics) {
624-
if (semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION)) {
625-
printer.append(getUnambiguousCanonicalForm());
626-
} else {
628+
if (getRepository().isMain()
629+
&& !semantics.getBool(
630+
BuildLanguageOptions.INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION)) {
631+
// If this label is in the main repo and we're not using unambiguous label stringification,
632+
// the result should always be "//foo:bar".
627633
printer.append(getCanonicalForm());
634+
return;
635+
}
636+
637+
if (semantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
638+
// If Bzlmod is enabled, we use canonical label literal syntax here and prepend an extra '@'.
639+
// So the result looks like "@@//foo:bar" for the main repo and "@@foo~1.0//bar:quux" for
640+
// other repos.
641+
printer.append('@');
628642
}
643+
// If Bzlmod is not enabled, we just use a single '@'.
644+
// So the result looks like "@//foo:bar" for the main repo and "@foo//bar:quux" for other repos.
645+
printer.append(getUnambiguousCanonicalForm());
629646
}
630647

631648
@Override

‎src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,20 @@ private LabelParser() {}
2828
*/
2929
static final class Parts {
3030
/**
31-
* The {@code @repo} part of the string (sans the {@literal @}); can be null if it doesn't have
32-
* such a part.
31+
* The {@code @repo} or {@code @@canonical_repo} part of the string (sans any leading
32+
* {@literal @}s); can be null if it doesn't have such a part (i.e. if it doesn't start with a
33+
* {@literal @}).
3334
*/
3435
@Nullable final String repo;
35-
/** Whether the package part of the string is prefixed by double-slash. */
36+
/**
37+
* Whether the repo part is using the canonical repo syntax (two {@literal @}s) or not (one
38+
* {@literal @}). If there is no repo part, this is false.
39+
*/
40+
final boolean repoIsCanonical;
41+
/**
42+
* Whether the package part of the string is prefixed by double-slash. This can only be false if
43+
* the repo part is missing.
44+
*/
3645
final boolean pkgIsAbsolute;
3746
/** The package part of the string (sans double-slash, if any). */
3847
final String pkg;
@@ -42,51 +51,77 @@ static final class Parts {
4251
final String raw;
4352

4453
private Parts(
45-
@Nullable String repo, boolean pkgIsAbsolute, String pkg, String target, String raw) {
54+
@Nullable String repo,
55+
boolean repoIsCanonical,
56+
boolean pkgIsAbsolute,
57+
String pkg,
58+
String target,
59+
String raw) {
4660
this.repo = repo;
61+
this.repoIsCanonical = repoIsCanonical;
4762
this.pkgIsAbsolute = pkgIsAbsolute;
4863
this.pkg = pkg;
4964
this.target = target;
5065
this.raw = raw;
5166
}
5267

5368
private static Parts validateAndCreate(
54-
@Nullable String repo, boolean pkgIsAbsolute, String pkg, String target, String raw)
69+
@Nullable String repo,
70+
boolean repoIsCanonical,
71+
boolean pkgIsAbsolute,
72+
String pkg,
73+
String target,
74+
String raw)
5575
throws LabelSyntaxException {
5676
validateRepoName(repo);
5777
validatePackageName(pkg, target);
58-
return new Parts(repo, pkgIsAbsolute, pkg, validateAndProcessTargetName(pkg, target), raw);
78+
return new Parts(
79+
repo,
80+
repoIsCanonical,
81+
pkgIsAbsolute,
82+
pkg,
83+
validateAndProcessTargetName(pkg, target),
84+
raw);
5985
}
6086

6187
/**
6288
* Parses a raw label string into parts. The logic can be summarized by the following table:
6389
*
6490
* {@code
65-
* raw | repo | pkgIsAbsolute | pkg | target
66-
* ---------------------+--------+---------------+-----------+-----------
67-
* foo/bar | null | false | "" | "foo/bar"
68-
* //foo/bar | null | true | "foo/bar" | "bar"
69-
* @repo | "repo" | true | "" | "repo"
70-
* @repo//foo/bar | "repo" | true | "foo/bar" | "bar"
71-
* :quux | null | false | "" | "quux"
72-
* foo/bar:quux | null | false | "foo/bar" | "quux"
73-
* //foo/bar:quux | null | true | "foo/bar" | "quux"
74-
* @repo//foo/bar:quux | "repo" | true | "foo/bar" | "quux"
91+
* raw | repo | repoIsCanonical | pkgIsAbsolute | pkg | target
92+
* ----------------------+--------+-----------------+---------------+-----------+-----------
93+
* foo/bar | null | false | false | "" | "foo/bar"
94+
* //foo/bar | null | false | true | "foo/bar" | "bar"
95+
* @repo | "repo" | false | true | "" | "repo"
96+
* @@repo | "repo" | true | true | "" | "repo"
97+
* @repo//foo/bar | "repo" | false | true | "foo/bar" | "bar"
98+
* @@repo//foo/bar | "repo" | true | true | "foo/bar" | "bar"
99+
* :quux | null | false | false | "" | "quux"
100+
* foo/bar:quux | null | false | false | "foo/bar" | "quux"
101+
* //foo/bar:quux | null | false | true | "foo/bar" | "quux"
102+
* @repo//foo/bar:quux | "repo" | false | true | "foo/bar" | "quux"
103+
* @@repo//foo/bar:quux | "repo" | true | true | "foo/bar" | "quux"
75104
* }
76105
*/
77106
static Parts parse(String rawLabel) throws LabelSyntaxException {
78107
@Nullable final String repo;
108+
final boolean repoIsCanonical = rawLabel.startsWith("@@");
79109
final int startOfPackage;
80110
final int doubleSlashIndex = rawLabel.indexOf("//");
81111
final boolean pkgIsAbsolute;
82112
if (rawLabel.startsWith("@")) {
83113
if (doubleSlashIndex < 0) {
84114
// Special case: the label "@foo" is synonymous with "@foo//:foo".
85-
repo = rawLabel.substring(1);
115+
repo = rawLabel.substring(repoIsCanonical ? 2 : 1);
86116
return validateAndCreate(
87-
repo, /*pkgIsAbsolute=*/ true, /*pkg=*/ "", /*target=*/ repo, rawLabel);
117+
repo,
118+
repoIsCanonical,
119+
/*pkgIsAbsolute=*/ true,
120+
/*pkg=*/ "",
121+
/*target=*/ repo,
122+
rawLabel);
88123
} else {
89-
repo = rawLabel.substring(1, doubleSlashIndex);
124+
repo = rawLabel.substring(repoIsCanonical ? 2 : 1, doubleSlashIndex);
90125
startOfPackage = doubleSlashIndex + 2;
91126
pkgIsAbsolute = true;
92127
}
@@ -114,7 +149,7 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
114149
pkg = "";
115150
target = rawLabel.substring(startOfPackage);
116151
}
117-
return validateAndCreate(repo, pkgIsAbsolute, pkg, target, rawLabel);
152+
return validateAndCreate(repo, repoIsCanonical, pkgIsAbsolute, pkg, target, rawLabel);
118153
}
119154

120155
private static void validateRepoName(@Nullable String repo) throws LabelSyntaxException {

‎src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,6 @@ public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMapp
8282
* provided apparent repo name is assumed to be valid.
8383
*/
8484
public RepositoryName get(String preMappingName) {
85-
if (preMappingName.startsWith("@")) {
86-
// The given name is actually a canonical, post-mapping repo name already.
87-
return RepositoryName.createUnvalidated(preMappingName);
88-
}
8985
RepositoryName canonicalRepoName = repositoryMapping().get(preMappingName);
9086
if (canonicalRepoName != null) {
9187
return canonicalRepoName;

‎src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public final class RepositoryName {
3939

4040
@SerializationConstant public static final RepositoryName MAIN = new RepositoryName("");
4141

42-
private static final Pattern VALID_REPO_NAME = Pattern.compile("@?[\\w\\-.~]*");
42+
private static final Pattern VALID_REPO_NAME = Pattern.compile("[\\w\\-.~]*");
4343

4444
// Must start with a letter. Can contain ASCII letters and digits, underscore, dash, and dot.
4545
private static final Pattern VALID_USER_PROVIDED_NAME = Pattern.compile("[a-zA-Z][-.\\w]*$");

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,13 @@ public void createValue_basic() throws Exception {
9393
.containsExactly(
9494
RepositoryName.MAIN,
9595
ModuleKey.ROOT,
96-
RepositoryName.create("@dep~1.0"),
96+
RepositoryName.create("dep~1.0"),
9797
createModuleKey("dep", "1.0"),
98-
RepositoryName.create("@dep~2.0"),
98+
RepositoryName.create("dep~2.0"),
9999
createModuleKey("dep", "2.0"),
100-
RepositoryName.create("@rules_cc~1.0"),
100+
RepositoryName.create("rules_cc~1.0"),
101101
createModuleKey("rules_cc", "1.0"),
102-
RepositoryName.create("@rules_java~override"),
102+
RepositoryName.create("rules_java~override"),
103103
createModuleKey("rules_java", ""));
104104
assertThat(value.getModuleNameLookup())
105105
.containsExactly(
@@ -181,10 +181,10 @@ public void createValue_moduleExtensions() throws Exception {
181181

182182
assertThat(value.getExtensionUniqueNames())
183183
.containsExactly(
184-
maven, "@rules_jvm_external~1.0~maven",
185-
pip, "@rules_python~2.0~pip",
186-
myext, "@dep~2.0~myext",
187-
myext2, "@dep~2.0~myext2");
184+
maven, "rules_jvm_external~1.0~maven",
185+
pip, "rules_python~2.0~pip",
186+
myext, "dep~2.0~myext",
187+
myext2, "dep~2.0~myext2");
188188

189189
assertThat(value.getFullRepoMapping(ModuleKey.ROOT))
190190
.isEqualTo(
@@ -195,26 +195,26 @@ public void createValue_moduleExtensions() throws Exception {
195195
"root",
196196
"",
197197
"rje",
198-
"@rules_jvm_external~1.0",
198+
"rules_jvm_external~1.0",
199199
"rpy",
200-
"@rules_python~2.0",
200+
"rules_python~2.0",
201201
"av",
202-
"@rules_jvm_external~1.0~maven~autovalue",
202+
"rules_jvm_external~1.0~maven~autovalue",
203203
"numpy",
204-
"@rules_python~2.0~pip~numpy"));
204+
"rules_python~2.0~pip~numpy"));
205205
assertThat(value.getFullRepoMapping(depKey))
206206
.isEqualTo(
207207
createRepositoryMapping(
208208
depKey,
209209
"dep",
210-
"@dep~2.0",
210+
"dep~2.0",
211211
"rules_python",
212-
"@rules_python~2.0",
212+
"rules_python~2.0",
213213
"np",
214-
"@rules_python~2.0~pip~numpy",
214+
"rules_python~2.0~pip~numpy",
215215
"oneext",
216-
"@dep~2.0~myext~myext",
216+
"dep~2.0~myext~myext",
217217
"twoext",
218-
"@dep~2.0~myext2~myext"));
218+
"dep~2.0~myext2~myext"));
219219
}
220220
}

‎src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleHelperTest.java

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -147,18 +147,18 @@ public void getRepoSpec_bazelModule() throws Exception {
147147
ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
148148

149149
EvaluationResult<GetRepoSpecByNameValue> result =
150-
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ccc~2.0")), evaluationContext);
150+
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ccc~2.0")), evaluationContext);
151151
if (result.hasError()) {
152152
fail(result.getError().toString());
153153
}
154154

155-
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~2.0")).rule();
155+
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~2.0")).rule();
156156
assertThat(repoSpec)
157157
.hasValue(
158158
RepoSpec.builder()
159159
.setRuleClassName("local_repository")
160160
.setAttributes(
161-
ImmutableMap.of("name", "@ccc~2.0", "path", "/usr/local/modules/@ccc~2.0"))
161+
ImmutableMap.of("name", "ccc~2.0", "path", "/usr/local/modules/ccc~2.0"))
162162
.build());
163163
}
164164

@@ -180,19 +180,19 @@ public void getRepoSpec_nonRegistryOverride() throws Exception {
180180

181181
EvaluationResult<GetRepoSpecByNameValue> result =
182182
evaluator.evaluate(
183-
ImmutableList.of(getRepoSpecByNameKey("@ccc~override")), evaluationContext);
183+
ImmutableList.of(getRepoSpecByNameKey("ccc~override")), evaluationContext);
184184
if (result.hasError()) {
185185
fail(result.getError().toString());
186186
}
187187

188-
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~override")).rule();
188+
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~override")).rule();
189189
assertThat(repoSpec)
190190
.hasValue(
191191
RepoSpec.builder()
192192
.setRuleClassName("local_repository")
193193
.setAttributes(
194194
ImmutableMap.of(
195-
"name", "@ccc~override",
195+
"name", "ccc~override",
196196
"path", "/foo/bar/C"))
197197
.build());
198198
}
@@ -216,12 +216,12 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
216216
ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
217217

218218
EvaluationResult<GetRepoSpecByNameValue> result =
219-
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ccc~3.0")), evaluationContext);
219+
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ccc~3.0")), evaluationContext);
220220
if (result.hasError()) {
221221
fail(result.getError().toString());
222222
}
223223

224-
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~3.0")).rule();
224+
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~3.0")).rule();
225225
assertThat(repoSpec)
226226
.hasValue(
227227
RepoSpec.builder()
@@ -232,9 +232,9 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
232232
.setAttributes(
233233
ImmutableMap.of(
234234
"name",
235-
"@ccc~3.0",
235+
"ccc~3.0",
236236
"path",
237-
"/usr/local/modules/@ccc~3.0",
237+
"/usr/local/modules/ccc~3.0",
238238
"patches",
239239
ImmutableList.of("//:foo.patch"),
240240
"patch_args",
@@ -264,18 +264,18 @@ public void getRepoSpec_multipleVersionOverride() throws Exception {
264264
ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
265265

266266
EvaluationResult<GetRepoSpecByNameValue> result =
267-
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ddd~2.0")), evaluationContext);
267+
evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ddd~2.0")), evaluationContext);
268268
if (result.hasError()) {
269269
fail(result.getError().toString());
270270
}
271271

272-
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ddd~2.0")).rule();
272+
Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ddd~2.0")).rule();
273273
assertThat(repoSpec)
274274
.hasValue(
275275
RepoSpec.builder()
276276
.setRuleClassName("local_repository")
277277
.setAttributes(
278-
ImmutableMap.of("name", "@ddd~2.0", "path", "/usr/local/modules/@ddd~2.0"))
278+
ImmutableMap.of("name", "ddd~2.0", "path", "/usr/local/modules/ddd~2.0"))
279279
.build());
280280
}
281281

0 commit comments

Comments
 (0)