Skip to content

Commit 4576e4a

Browse files
Wyveraldfmeum
Wyverald
and
fmeum
authored
[7.1.0] Add Label.to_display_form() (bazelbuild#21312)
Fixes bazelbuild#20486 While `to_display_form()` can be called in all contexts, it only returns apparent names for BUILD threads, which are the most common use case for display form labels. Support for module extensions can be added later, but requires explicit tracking of inverse mappings in the lockfile. 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. Closes bazelbuild#21179. PiperOrigin-RevId: 606330539 Change-Id: Id6a4ad79fd3d50319320789b88c618aad28db28b Co-authored-by: Fabian Meumertzheim <[email protected]>
1 parent 1991f52 commit 4576e4a

File tree

17 files changed

+329
-14
lines changed

17 files changed

+329
-14
lines changed

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

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -373,13 +373,9 @@ private String getProgressMessageChecked(@Nullable RepositoryMapping mainReposit
373373
private String replaceProgressMessagePlaceholders(
374374
String progressMessage, @Nullable RepositoryMapping mainRepositoryMapping) {
375375
if (progressMessage.contains("%{label}") && owner.getLabel() != null) {
376-
String labelString;
377-
if (mainRepositoryMapping != null) {
378-
labelString = owner.getLabel().getDisplayForm(mainRepositoryMapping);
379-
} else {
380-
labelString = owner.getLabel().toString();
381-
}
382-
progressMessage = progressMessage.replace("%{label}", labelString);
376+
progressMessage =
377+
progressMessage.replace(
378+
"%{label}", owner.getLabel().getDisplayForm(mainRepositoryMapping));
383379
}
384380
if (progressMessage.contains("%{output}") && getPrimaryOutput() != null) {
385381
progressMessage =

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

Lines changed: 9 additions & 1 deletion
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‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,10 +437,28 @@ public String getUnambiguousCanonicalForm() {
437437
* @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
438438
* @return analogous to {@link PackageIdentifier#getDisplayForm(RepositoryMapping)}
439439
*/
440-
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
440+
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
441441
return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
442442
}
443443

444+
@StarlarkMethod(
445+
name = "to_display_form",
446+
useStarlarkThread = true,
447+
doc =
448+
"Returns a string representation of this label that is optimized for human readability."
449+
+ " Use this to format a <code>Label</code> for use in BUILD files. <p>The exact form"
450+
+ " of the return value is explicitly unspecified and subject to change. The"
451+
+ " following properties are guaranteed for a <code>Label</code> <code>l</code>:<ul> "
452+
+ " <li><code>l.to_display_form()</code> has no repository part if and only if"
453+
+ " <code>l</code> references the main repository;</li> "
454+
+ " <li><code>Label(l.to_display_form()) == l</code> if the call to"
455+
+ " <code>Label</code> occurs in the main repository.</li></ul>")
456+
public String getDisplayFormForStarlark(StarlarkThread starlarkThread) throws EvalException {
457+
checkRepoVisibilityForStarlark("to_display_form");
458+
return getDisplayForm(
459+
BazelModuleContext.ofInnermostBzlOrThrow(starlarkThread).bestEffortMainRepoMapping());
460+
}
461+
444462
/**
445463
* Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying
446464
* 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‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.devtools.build.lib.vfs.PathFragment;
2323
import com.google.devtools.build.skyframe.SkyFunctionName;
2424
import com.google.devtools.build.skyframe.SkyKey;
25+
import javax.annotation.Nullable;
2526
import javax.annotation.concurrent.Immutable;
2627

2728
/**
@@ -215,7 +216,7 @@ public String getUnambiguousCanonicalForm() {
215216
* <dd>only with Bzlmod if the current package belongs to a repository that is not visible
216217
* from the main module
217218
*/
218-
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
219+
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
219220
return repository.getDisplayForm(mainRepositoryMapping) + "//" + pkgName;
220221
}
221222

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

Lines changed: 8 additions & 3 deletions
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‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
import java.util.LinkedHashSet;
6969
import java.util.List;
7070
import java.util.Map;
71+
import java.util.Optional;
7172
import java.util.concurrent.atomic.AtomicBoolean;
7273
import java.util.function.Consumer;
7374
import javax.annotation.Nullable;
@@ -765,6 +766,11 @@ private BzlLoadValue computeInternalWithCompiledBzl(
765766
if (repoMapping == null) {
766767
return null;
767768
}
769+
Optional<RepositoryMapping> mainRepoMapping =
770+
getMainRepositoryMapping(key, builtins.starlarkSemantics, env);
771+
if (mainRepoMapping == null) {
772+
return null;
773+
}
768774
Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder();
769775
ImmutableList<Pair<String, Location>> programLoads = getLoadsFromProgram(prog);
770776
ImmutableList<Label> loadLabels =
@@ -843,6 +849,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
843849
BazelModuleContext.create(
844850
label,
845851
repoMapping,
852+
mainRepoMapping.orElse(null),
846853
prog.getFilename(),
847854
ImmutableList.copyOf(loadMap.values()),
848855
transitiveDigest);
@@ -977,6 +984,34 @@ private static RepositoryMapping getRepositoryMapping(
977984
return repositoryMappingValue.getRepositoryMapping();
978985
}
979986

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

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

Lines changed: 1 addition & 1 deletion
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2533,4 +2533,37 @@ public void innate_noSuchValue() throws Exception {
25332533
"//:repo.bzl does not export a repository_rule called data_repo, yet its use is"
25342534
+ " requested at /ws/MODULE.bazel");
25352535
}
2536+
2537+
@Test
2538+
public void labelToDisplayForm() throws Exception {
2539+
scratch.file(
2540+
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
2541+
"bazel_dep(name='data_repo', version='1.0')",
2542+
"ext = use_extension('//:defs.bzl', 'ext')",
2543+
"use_repo(ext, 'foo', 'bar', 'baz')");
2544+
scratch.file(
2545+
workspaceRoot.getRelative("defs.bzl").getPathString(),
2546+
"load('@data_repo//:defs.bzl','data_repo')",
2547+
"def _ext_impl(ctx):",
2548+
" data_repo(name='foo',data=Label('//:foo').to_display_form())",
2549+
" data_repo(name='bar',data=Label('@data_repo//:bar').to_display_form())",
2550+
" data_repo(name='baz',data=Label('@@canonical_name//:baz').to_display_form())",
2551+
"ext = module_extension(implementation=_ext_impl)");
2552+
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
2553+
scratch.file(
2554+
workspaceRoot.getRelative("data.bzl").getPathString(),
2555+
"load('@foo//:data.bzl', foo_data='data')",
2556+
"load('@bar//:data.bzl', bar_data='data')",
2557+
"load('@baz//:data.bzl', baz_data='data')",
2558+
"data = 'foo:'+foo_data+' bar:'+bar_data+' baz:'+baz_data");
2559+
2560+
SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
2561+
EvaluationResult<BzlLoadValue> result =
2562+
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
2563+
if (result.hasError()) {
2564+
throw result.getError().getException();
2565+
}
2566+
assertThat(result.get(skyKey).getModule().getGlobal("data"))
2567+
.isEqualTo("foo://:foo bar:@@data_repo~//:bar baz:@@canonical_name//:baz");
2568+
}
25362569
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,12 @@ public void testDisplayForm() throws Exception {
418418
.isEqualTo("@unremapped//:unremapped");
419419
}
420420

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

0 commit comments

Comments
 (0)