Skip to content

Commit 3da0c80

Browse files
Wyveraldaiuto
Wyverald
authored and
aiuto
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. (bazelbuild@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 bazelbuild#15916 PiperOrigin-RevId: 469949029 Change-Id: Id77dc13752e9d945a99a823e91a2c4d9d8342623
1 parent 80d977d commit 3da0c80

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)