Skip to content

Commit

Permalink
Add Label.to_display_form()
Browse files Browse the repository at this point in the history
Fixes #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 #21179.

PiperOrigin-RevId: 606330539
Change-Id: Id6a4ad79fd3d50319320789b88c618aad28db28b
  • Loading branch information
fmeum authored and Wyverald committed Feb 13, 2024
1 parent 1991f52 commit 0655754
Show file tree
Hide file tree
Showing 17 changed files with 329 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,9 @@ private String getProgressMessageChecked(@Nullable RepositoryMapping mainReposit
private String replaceProgressMessagePlaceholders(
String progressMessage, @Nullable RepositoryMapping mainRepositoryMapping) {
if (progressMessage.contains("%{label}") && owner.getLabel() != null) {
String labelString;
if (mainRepositoryMapping != null) {
labelString = owner.getLabel().getDisplayForm(mainRepositoryMapping);
} else {
labelString = owner.getLabel().toString();
}
progressMessage = progressMessage.replace("%{label}", labelString);
progressMessage =
progressMessage.replace(
"%{label}", owner.getLabel().getDisplayForm(mainRepositoryMapping));
}
if (progressMessage.contains("%{output}") && getPrimaryOutput() != null) {
progressMessage =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ public abstract class BazelModuleContext {
/** The repository mapping applicable to the repo where the .bzl file is located in. */
public abstract RepositoryMapping repoMapping();

/**
* The repository mapping applicable to the main repository, possibly without WORKSPACE repos or
* null. This is purely meant to support {@link Label#getDisplayFormForStarlark(StarlarkThread)}.
*/
@Nullable
public abstract RepositoryMapping bestEffortMainRepoMapping();

/** Returns the name of the module's .bzl file, as provided to the parser. */
public abstract String filename();

Expand Down Expand Up @@ -160,11 +167,12 @@ public static BazelModuleContext ofInnermostBzlOrFail(StarlarkThread thread, Str
public static BazelModuleContext create(
Label label,
RepositoryMapping repoMapping,
@Nullable RepositoryMapping bestEffortMainRepoMapping,
String filename,
ImmutableList<Module> loads,
byte[] bzlTransitiveDigest) {
return new AutoValue_BazelModuleContext(
label, repoMapping, filename, loads, bzlTransitiveDigest);
label, repoMapping, bestEffortMainRepoMapping, filename, loads, bzlTransitiveDigest);
}

public final Label.PackageContext packageContext() {
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/google/devtools/build/lib/cmdline/Label.java
Original file line number Diff line number Diff line change
Expand Up @@ -437,10 +437,28 @@ public String getUnambiguousCanonicalForm() {
* @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
* @return analogous to {@link PackageIdentifier#getDisplayForm(RepositoryMapping)}
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
}

@StarlarkMethod(
name = "to_display_form",
useStarlarkThread = true,
doc =
"Returns a string representation of this label that is optimized for human readability."
+ " Use this to format a <code>Label</code> for use in BUILD files. <p>The exact form"
+ " of the return value is explicitly unspecified and subject to change. The"
+ " following properties are guaranteed for a <code>Label</code> <code>l</code>:<ul> "
+ " <li><code>l.to_display_form()</code> has no repository part if and only if"
+ " <code>l</code> references the main repository;</li> "
+ " <li><code>Label(l.to_display_form()) == l</code> if the call to"
+ " <code>Label</code> occurs in the main repository.</li></ul>")
public String getDisplayFormForStarlark(StarlarkThread starlarkThread) throws EvalException {
checkRepoVisibilityForStarlark("to_display_form");
return getDisplayForm(
BazelModuleContext.ofInnermostBzlOrThrow(starlarkThread).bestEffortMainRepoMapping());
}

/**
* Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying
* the repository part, labels of the form {@code [@repo]//foo/bar:bar} are simplified to the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -215,7 +216,7 @@ public String getUnambiguousCanonicalForm() {
* <dd>only with Bzlmod if the current package belongs to a repository that is not visible
* from the main module
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
return repository.getDisplayForm(mainRepositoryMapping) + "//" + pkgName;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,20 +247,25 @@ public String getCanonicalForm() {
* <dt><code>@protobuf</code>
* <dd>if this repository is a WORKSPACE dependency and its <code>name</code> is "protobuf",
* or if this repository is a Bzlmod dependency of the main module and its apparent name
* is "protobuf"
* is "protobuf" (in both cases only if mainRepositoryMapping is not null)
* <dt><code>@@protobuf~3.19.2</code>
* <dd>only with Bzlmod, if this a repository that is not visible from the main module
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
Preconditions.checkArgument(
mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
mainRepositoryMapping == null
|| mainRepositoryMapping.ownerRepo() == null
|| mainRepositoryMapping.ownerRepo().isMain());
if (!isVisible()) {
return getNameWithAt();
}
if (isMain()) {
// Packages in the main repository can always use repo-relative form.
return "";
}
if (mainRepositoryMapping == null) {
return getNameWithAt();
}
if (!mainRepositoryMapping.usesStrictDeps()) {
// If the main repository mapping is not using strict visibility, then Bzlmod is certainly
// disabled, which means that canonical and apparent names can be used interchangeably from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Consumer;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -765,6 +766,11 @@ private BzlLoadValue computeInternalWithCompiledBzl(
if (repoMapping == null) {
return null;
}
Optional<RepositoryMapping> mainRepoMapping =
getMainRepositoryMapping(key, builtins.starlarkSemantics, env);
if (mainRepoMapping == null) {
return null;
}
Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder();
ImmutableList<Pair<String, Location>> programLoads = getLoadsFromProgram(prog);
ImmutableList<Label> loadLabels =
Expand Down Expand Up @@ -843,6 +849,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
BazelModuleContext.create(
label,
repoMapping,
mainRepoMapping.orElse(null),
prog.getFilename(),
ImmutableList.copyOf(loadMap.values()),
transitiveDigest);
Expand Down Expand Up @@ -977,6 +984,34 @@ private static RepositoryMapping getRepositoryMapping(
return repositoryMappingValue.getRepositoryMapping();
}

@Nullable
private static Optional<RepositoryMapping> getMainRepositoryMapping(
BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env)
throws InterruptedException {
if (!starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
return Optional.empty();
}
RepositoryMappingValue.Key repoMappingKey;
// When adding cases for other key types such as WORKSPACE or Bzlmod, make sure to track the
// usages of the repo mapping in persistent caches, such as repository marker files and the
// MODULE.bazel.lock file.
if (key instanceof BzlLoadValue.KeyForBuild) {
repoMappingKey = RepositoryMappingValue.key(RepositoryName.MAIN);
} else if (key instanceof BzlLoadValue.KeyForBuiltins) {
// Using the full main repo mapping here results in a cycle as it depends on WORKSPACE, but
// builtins are injected into WORKSPACE. Fixing this fully would require adding a new key type
// for builtins (transitively) loaded from WORKSPACE.
repoMappingKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
} else {
return Optional.empty();
}
var mainRepositoryMappingValue = (RepositoryMappingValue) env.getValue(repoMappingKey);
if (mainRepositoryMappingValue == null) {
return null;
}
return Optional.of(mainRepositoryMappingValue.getRepositoryMapping());
}

/**
* Validates a label appearing in a {@code load()} statement, throwing {@link
* LabelSyntaxException} on failure.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ def _init_cc_compilation_context(
if not module_map:
module_map = cc_common.create_module_map(
file = actions.declare_file(label.name + ".cppmap"),
name = label.workspace_name + "//" + label.package + ":" + label.name,
name = label.to_display_form(),
)

# There are different modes for module compilation:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ private static Object exec(String... lines) throws Exception {
BazelModuleContext.create(
Label.parseCanonicalUnchecked("//test:label"),
RepositoryMapping.ALWAYS_FALLBACK,
/* bestEffortMainRepoMapping= */ null,
"test/label.bzl",
/* loads= */ ImmutableList.of(),
/* bzlTransitiveDigest= */ new byte[0])),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2533,4 +2533,37 @@ public void innate_noSuchValue() throws Exception {
"//:repo.bzl does not export a repository_rule called data_repo, yet its use is"
+ " requested at /ws/MODULE.bazel");
}

@Test
public void labelToDisplayForm() throws Exception {
scratch.file(
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
"bazel_dep(name='data_repo', version='1.0')",
"ext = use_extension('//:defs.bzl', 'ext')",
"use_repo(ext, 'foo', 'bar', 'baz')");
scratch.file(
workspaceRoot.getRelative("defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
"def _ext_impl(ctx):",
" data_repo(name='foo',data=Label('//:foo').to_display_form())",
" data_repo(name='bar',data=Label('@data_repo//:bar').to_display_form())",
" data_repo(name='baz',data=Label('@@canonical_name//:baz').to_display_form())",
"ext = module_extension(implementation=_ext_impl)");
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("data.bzl").getPathString(),
"load('@foo//:data.bzl', foo_data='data')",
"load('@bar//:data.bzl', bar_data='data')",
"load('@baz//:data.bzl', baz_data='data')",
"data = 'foo:'+foo_data+' bar:'+bar_data+' baz:'+baz_data");

SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
EvaluationResult<BzlLoadValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
if (result.hasError()) {
throw result.getError().getException();
}
assertThat(result.get(skyKey).getModule().getGlobal("data"))
.isEqualTo("foo://:foo bar:@@data_repo~//:bar baz:@@canonical_name//:baz");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,12 @@ public void testDisplayForm() throws Exception {
.isEqualTo("@unremapped//:unremapped");
}

@Test
public void testDisplayFormNullMapping() throws Exception {
assertThat(displayFormFor("//foo/bar:bar", null)).isEqualTo("//foo/bar:bar");
assertThat(displayFormFor("@@foo//bar:bar", null)).isEqualTo("@@foo//bar:bar");
}

private static String shorthandDisplayFormFor(
String rawLabel, RepositoryMapping repositoryMapping) throws Exception {
return Label.parseCanonical(rawLabel).getShorthandDisplayForm(repositoryMapping);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public void testDisplayFormInMainRepository() throws Exception {
RepositoryMapping.create(
ImmutableMap.of("foo", RepositoryName.create("bar")), RepositoryName.MAIN)))
.isEqualTo("//some/pkg");
assertThat(pkg.getDisplayForm(null)).isEqualTo("//some/pkg");
}

@Test
Expand All @@ -139,5 +140,6 @@ public void testDisplayFormInExternalRepository() throws Exception {
ImmutableMap.of("local", RepositoryName.create("other_repo")),
RepositoryName.MAIN)))
.isEqualTo("@@canonical//some/pkg");
assertThat(pkg.getDisplayForm(null)).isEqualTo("@@canonical//some/pkg");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,21 @@ public void testGetDisplayForm() throws Exception {
.getDisplayForm(repositoryMapping))
.isEqualTo("@@[unknown repo 'local' requested from @@owner]");
}

@Test
public void testGetDisplayFormWithNullMapping() throws Exception {
assertThat(RepositoryName.create("").getDisplayForm(null)).isEmpty();
assertThat(RepositoryName.create("canonical").getDisplayForm(null)).isEqualTo("@@canonical");

assertThat(
RepositoryName.create("")
.toNonVisible(RepositoryName.create("owner"))
.getDisplayForm(null))
.isEqualTo("@@[unknown repo '' requested from @@owner]");
assertThat(
RepositoryName.create("canonical")
.toNonVisible(RepositoryName.create("owner"))
.getDisplayForm(null))
.isEqualTo("@@[unknown repo 'canonical' requested from @@owner]");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4824,6 +4824,7 @@ public void testLabelWithStrictVisibility() throws Exception {
bzlLabel,
RepositoryMapping.create(
ImmutableMap.of("my_module", currentRepo, "dep", otherRepo), currentRepo),
/* bestEffortMainRepoMapping= */ null,
"lib/label.bzl",
/* loads= */ ImmutableList.of(),
/* bzlTransitiveDigest= */ new byte[0]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ private Object newModule(ImmutableMap.Builder<String, Object> predeclared) {
return BazelModuleContext.create(
label,
RepositoryMapping.ALWAYS_FALLBACK,
/* bestEffortMainRepoMapping= */ null,
"test/label.bzl",
/* loads= */ ImmutableList.of(),
/* bzlTransitiveDigest= */ new byte[0]);
Expand Down
Loading

0 comments on commit 0655754

Please sign in to comment.