Skip to content

Commit 14ce414

Browse files
committed
Add Label.to_display_form()
Fixes bazelbuild#20486 Also use `to_display_form()` to generate Clang module names in the correct form for external repositories. `java_*` rules require more delicate handling and will be migrated in a follow-up change. RELNOTES: The `to_display_form()` method on `Label` returns a string representation of a label optimized for readability by humans.
1 parent 65b1a69 commit 14ce414

17 files changed

+318
-14
lines changed

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

+3-7
Original file line numberDiff line numberDiff line change
@@ -368,13 +368,9 @@ private String getProgressMessageChecked(@Nullable RepositoryMapping mainReposit
368368
private String replaceProgressMessagePlaceholders(
369369
String progressMessage, @Nullable RepositoryMapping mainRepositoryMapping) {
370370
if (progressMessage.contains("%{label}") && owner.getLabel() != null) {
371-
String labelString;
372-
if (mainRepositoryMapping != null) {
373-
labelString = owner.getLabel().getDisplayForm(mainRepositoryMapping);
374-
} else {
375-
labelString = owner.getLabel().toString();
376-
}
377-
progressMessage = progressMessage.replace("%{label}", labelString);
371+
progressMessage =
372+
progressMessage.replace(
373+
"%{label}", owner.getLabel().getDisplayForm(mainRepositoryMapping));
378374
}
379375
if (progressMessage.contains("%{output}") && getPrimaryOutput() != null) {
380376
progressMessage =

src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java

+9-1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,13 @@ public abstract class BazelModuleContext {
4444
/** The repository mapping applicable to the repo where the .bzl file is located in. */
4545
public abstract RepositoryMapping repoMapping();
4646

47+
/**
48+
* The repository mapping applicable to the main repository, possibly without WORKSPACE repos or
49+
* null. This is purely meant to support {@link Label#getDisplayFormForStarlark(StarlarkThread)}.
50+
*/
51+
@Nullable
52+
public abstract RepositoryMapping bestEffortMainRepoMapping();
53+
4754
/** Returns the name of the module's .bzl file, as provided to the parser. */
4855
public abstract String filename();
4956

@@ -160,11 +167,12 @@ public static BazelModuleContext ofInnermostBzlOrFail(StarlarkThread thread, Str
160167
public static BazelModuleContext create(
161168
Label label,
162169
RepositoryMapping repoMapping,
170+
@Nullable RepositoryMapping bestEffortMainRepoMapping,
163171
String filename,
164172
ImmutableList<Module> loads,
165173
byte[] bzlTransitiveDigest) {
166174
return new AutoValue_BazelModuleContext(
167-
label, repoMapping, filename, loads, bzlTransitiveDigest);
175+
label, repoMapping, bestEffortMainRepoMapping, filename, loads, bzlTransitiveDigest);
168176
}
169177

170178
public final Label.PackageContext packageContext() {

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

+20-1
Original file line numberDiff line numberDiff line change
@@ -442,10 +442,29 @@ public String getUnambiguousCanonicalForm() {
442442
* @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
443443
* @return analogous to {@link PackageIdentifier#getDisplayForm(RepositoryMapping)}
444444
*/
445-
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
445+
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
446446
return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
447447
}
448448

449+
@StarlarkMethod(
450+
name = "to_display_form",
451+
useStarlarkThread = true,
452+
doc =
453+
"Returns a string representation of this label that is optimized for human readability."
454+
+ " Use this to format a <code>Label</code> for use in BUILD files."
455+
+ " <p>The exact form of the return value is explicitly unspecified and subject to"
456+
+ " change."
457+
+ " The following properties are guaranteed for a <code>Label</code> <code>l</code>:"
458+
+ "<ul>"
459+
+ " <li><code>l.to_display_form()</code> has no repository part if and only if <code>l</code> references the main repository;</li>"
460+
+ " <li><code>Label(l.to_display_form()) == l</code> if the call to <code>Label</code> occurs in the main repository.</li>"
461+
+ "</ul>")
462+
public String getDisplayFormForStarlark(StarlarkThread starlarkThread) throws EvalException {
463+
checkRepoVisibilityForStarlark("to_display_form");
464+
return getDisplayForm(
465+
BazelModuleContext.ofInnermostBzlOrThrow(starlarkThread).bestEffortMainRepoMapping());
466+
}
467+
449468
/**
450469
* Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying
451470
* the repository part, labels of the form {@code [@repo]//foo/bar:bar} are simplified to the

src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import com.google.devtools.build.skyframe.SkyFunctionName;
2525
import com.google.devtools.build.skyframe.SkyKey;
2626
import com.google.devtools.build.skyframe.SkyKey.SkyKeyInterner;
27+
28+
import javax.annotation.Nullable;
2729
import javax.annotation.concurrent.Immutable;
2830

2931
/**
@@ -226,7 +228,7 @@ public String getUnambiguousCanonicalForm() {
226228
* <dd>only with Bzlmod if the current package belongs to a repository that is not visible
227229
* from the main module
228230
*/
229-
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
231+
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
230232
return repository.getDisplayForm(mainRepositoryMapping) + "//" + pkgName;
231233
}
232234

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

+8-3
Original file line numberDiff line numberDiff line change
@@ -247,20 +247,25 @@ public String getCanonicalForm() {
247247
* <dt><code>@protobuf</code>
248248
* <dd>if this repository is a WORKSPACE dependency and its <code>name</code> is "protobuf",
249249
* or if this repository is a Bzlmod dependency of the main module and its apparent name
250-
* is "protobuf"
250+
* is "protobuf" (in both cases only if mainRepositoryMapping is not null)
251251
* <dt><code>@@protobuf~3.19.2</code>
252252
* <dd>only with Bzlmod, if this a repository that is not visible from the main module
253253
*/
254-
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
254+
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
255255
Preconditions.checkArgument(
256-
mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
256+
mainRepositoryMapping == null
257+
|| mainRepositoryMapping.ownerRepo() == null
258+
|| mainRepositoryMapping.ownerRepo().isMain());
257259
if (!isVisible()) {
258260
return getNameWithAt();
259261
}
260262
if (isMain()) {
261263
// Packages in the main repository can always use repo-relative form.
262264
return "";
263265
}
266+
if (mainRepositoryMapping == null) {
267+
return getNameWithAt();
268+
}
264269
if (!mainRepositoryMapping.usesStrictDeps()) {
265270
// If the main repository mapping is not using strict visibility, then Bzlmod is certainly
266271
// disabled, which means that canonical and apparent names can be used interchangeably from

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

+35
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
import java.util.LinkedHashSet;
6767
import java.util.List;
6868
import java.util.Map;
69+
import java.util.Optional;
6970
import java.util.concurrent.atomic.AtomicBoolean;
7071
import java.util.function.Consumer;
7172
import javax.annotation.Nullable;
@@ -762,6 +763,11 @@ private BzlLoadValue computeInternalWithCompiledBzl(
762763
if (repoMapping == null) {
763764
return null;
764765
}
766+
Optional<RepositoryMapping> mainRepoMapping =
767+
getMainRepositoryMapping(key, builtins.starlarkSemantics, env);
768+
if (mainRepoMapping == null) {
769+
return null;
770+
}
765771
Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder();
766772
ImmutableList<Pair<String, Location>> programLoads = getLoadsFromProgram(prog);
767773
ImmutableList<Label> loadLabels =
@@ -840,6 +846,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
840846
BazelModuleContext.create(
841847
label,
842848
repoMapping,
849+
mainRepoMapping.orElse(null),
843850
prog.getFilename(),
844851
ImmutableList.copyOf(loadMap.values()),
845852
transitiveDigest);
@@ -974,6 +981,34 @@ private static RepositoryMapping getRepositoryMapping(
974981
return repositoryMappingValue.getRepositoryMapping();
975982
}
976983

984+
@Nullable
985+
private static Optional<RepositoryMapping> getMainRepositoryMapping(
986+
BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env)
987+
throws InterruptedException {
988+
if (!starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
989+
return Optional.empty();
990+
}
991+
RepositoryMappingValue.Key repoMappingKey;
992+
// When adding cases for other key types such as WORKSPACE or Bzlmod, make sure to track the
993+
// usages of the repo mapping in persistent caches, such as repository marker files and the
994+
// MODULE.bazel.lock file.
995+
if (key instanceof BzlLoadValue.KeyForBuild) {
996+
repoMappingKey = RepositoryMappingValue.key(RepositoryName.MAIN);
997+
} else if (key instanceof BzlLoadValue.KeyForBuiltins) {
998+
// Using the full main repo mapping here results in a cycle as it depends on WORKSPACE, but
999+
// builtins are injected into WORKSPACE. Fixing this fully would require adding a new key type
1000+
// for builtins (transitively) loaded from WORKSPACE.
1001+
repoMappingKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
1002+
} else {
1003+
return Optional.empty();
1004+
}
1005+
var mainRepositoryMappingValue = (RepositoryMappingValue) env.getValue(repoMappingKey);
1006+
if (mainRepositoryMappingValue == null) {
1007+
return null;
1008+
}
1009+
return Optional.of(mainRepositoryMappingValue.getRepositoryMapping());
1010+
}
1011+
9771012
/**
9781013
* Validates a label appearing in a {@code load()} statement, throwing {@link
9791014
* LabelSyntaxException} on failure.

src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl

+1-1
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ def _init_cc_compilation_context(
297297
if not module_map:
298298
module_map = cc_common.create_module_map(
299299
file = actions.declare_file(label.name + ".cppmap"),
300-
name = label.workspace_name + "//" + label.package + ":" + label.name,
300+
name = label.to_display_form(),
301301
)
302302

303303
# There are different modes for module compilation:

src/test/java/com/google/devtools/build/lib/analysis/actions/BuildInfoFileWriteActionTest.java

+1
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ private static Object exec(String... lines) throws Exception {
7878
BazelModuleContext.create(
7979
Label.parseCanonicalUnchecked("//test:label"),
8080
RepositoryMapping.ALWAYS_FALLBACK,
81+
/* bestEffortMainRepoMapping= */ null,
8182
"test/label.bzl",
8283
/* loads= */ ImmutableList.of(),
8384
/* bzlTransitiveDigest= */ new byte[0])),

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

+33
Original file line numberDiff line numberDiff line change
@@ -2518,4 +2518,37 @@ public void innate_noSuchValue() throws Exception {
25182518
"//:repo.bzl does not export a repository_rule called data_repo, yet its use is"
25192519
+ " requested at /ws/MODULE.bazel");
25202520
}
2521+
2522+
@Test
2523+
public void labelToDisplayForm() throws Exception {
2524+
scratch.file(
2525+
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
2526+
"bazel_dep(name='data_repo', version='1.0')",
2527+
"ext = use_extension('//:defs.bzl', 'ext')",
2528+
"use_repo(ext, 'foo', 'bar', 'baz')");
2529+
scratch.file(
2530+
workspaceRoot.getRelative("defs.bzl").getPathString(),
2531+
"load('@data_repo//:defs.bzl','data_repo')",
2532+
"def _ext_impl(ctx):",
2533+
" data_repo(name='foo',data=Label('//:foo').to_display_form())",
2534+
" data_repo(name='bar',data=Label('@data_repo//:bar').to_display_form())",
2535+
" data_repo(name='baz',data=Label('@@canonical_name//:baz').to_display_form())",
2536+
"ext = module_extension(implementation=_ext_impl)");
2537+
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
2538+
scratch.file(
2539+
workspaceRoot.getRelative("data.bzl").getPathString(),
2540+
"load('@foo//:data.bzl', foo_data='data')",
2541+
"load('@bar//:data.bzl', bar_data='data')",
2542+
"load('@baz//:data.bzl', baz_data='data')",
2543+
"data = 'foo:'+foo_data+' bar:'+bar_data+' baz:'+baz_data");
2544+
2545+
SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
2546+
EvaluationResult<BzlLoadValue> result =
2547+
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
2548+
if (result.hasError()) {
2549+
throw result.getError().getException();
2550+
}
2551+
assertThat(result.get(skyKey).getModule().getGlobal("data"))
2552+
.isEqualTo("foo://:foo bar:@@data_repo~1.0//:bar baz:@@canonical_name//:baz");
2553+
}
25212554
}

src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java

+6
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,12 @@ public void testDisplayForm() throws Exception {
419419
.isEqualTo("@unremapped//:unremapped");
420420
}
421421

422+
@Test
423+
public void testDisplayFormNullMapping() throws Exception {
424+
assertThat(displayFormFor("//foo/bar:bar", null)).isEqualTo("//foo/bar:bar");
425+
assertThat(displayFormFor("@@foo//bar:bar", null)).isEqualTo("@@foo//bar:bar");
426+
}
427+
422428
private static String shorthandDisplayFormFor(
423429
String rawLabel, RepositoryMapping repositoryMapping) throws Exception {
424430
return Label.parseCanonical(rawLabel).getShorthandDisplayForm(repositoryMapping);

src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java

+2
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ public void testDisplayFormInMainRepository() throws Exception {
120120
RepositoryMapping.create(
121121
ImmutableMap.of("foo", RepositoryName.create("bar")), RepositoryName.MAIN)))
122122
.isEqualTo("//some/pkg");
123+
assertThat(pkg.getDisplayForm(null)).isEqualTo("//some/pkg");
123124
}
124125

125126
@Test
@@ -139,5 +140,6 @@ public void testDisplayFormInExternalRepository() throws Exception {
139140
ImmutableMap.of("local", RepositoryName.create("other_repo")),
140141
RepositoryName.MAIN)))
141142
.isEqualTo("@@canonical//some/pkg");
143+
assertThat(pkg.getDisplayForm(null)).isEqualTo("@@canonical//some/pkg");
142144
}
143145
}

src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java

+18
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,22 @@ public void testGetDisplayForm() throws Exception {
105105
.getDisplayForm(repositoryMapping))
106106
.isEqualTo("@@[unknown repo 'local' requested from @@owner]");
107107
}
108+
109+
@Test
110+
public void testGetDisplayFormWithNullMapping() throws Exception {
111+
assertThat(RepositoryName.create("").getDisplayForm(null)).isEmpty();
112+
assertThat(RepositoryName.create("canonical").getDisplayForm(null))
113+
.isEqualTo("@@canonical");
114+
115+
assertThat(
116+
RepositoryName.create("")
117+
.toNonVisible(RepositoryName.create("owner"))
118+
.getDisplayForm(null))
119+
.isEqualTo("@@[unknown repo '' requested from @@owner]");
120+
assertThat(
121+
RepositoryName.create("canonical")
122+
.toNonVisible(RepositoryName.create("owner"))
123+
.getDisplayForm(null))
124+
.isEqualTo("@@[unknown repo 'canonical' requested from @@owner]");
125+
}
108126
}

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java

+1
Original file line numberDiff line numberDiff line change
@@ -5047,6 +5047,7 @@ public void testLabelWithStrictVisibility() throws Exception {
50475047
bzlLabel,
50485048
RepositoryMapping.create(
50495049
ImmutableMap.of("my_module", currentRepo, "dep", otherRepo), currentRepo),
5050+
/* bestEffortMainRepoMapping= */ null,
50505051
"lib/label.bzl",
50515052
/* loads= */ ImmutableList.of(),
50525053
/* bzlTransitiveDigest= */ new byte[0]);

src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java

+1
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ private Object newModule(ImmutableMap.Builder<String, Object> predeclared) {
175175
return BazelModuleContext.create(
176176
label,
177177
RepositoryMapping.ALWAYS_FALLBACK,
178+
/* bestEffortMainRepoMapping= */ null,
178179
"test/label.bzl",
179180
/* loads= */ ImmutableList.of(),
180181
/* bzlTransitiveDigest= */ new byte[0]);

0 commit comments

Comments
 (0)