From 4576e4a8a4055e3ddfe0ef1c7c10c4cb1cc7cc21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Tue, 13 Feb 2024 03:54:19 +0100 Subject: [PATCH] [7.1.0] Add `Label.to_display_form()` (#21312) 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 Co-authored-by: Fabian Meumertzheim --- .../build/lib/actions/AbstractAction.java | 10 +- .../build/lib/cmdline/BazelModuleContext.java | 10 +- .../devtools/build/lib/cmdline/Label.java | 20 ++- .../build/lib/cmdline/PackageIdentifier.java | 3 +- .../build/lib/cmdline/RepositoryName.java | 11 +- .../build/lib/skyframe/BzlLoadFunction.java | 35 +++++ .../common/cc/cc_compilation_helper.bzl | 2 +- .../actions/BuildInfoFileWriteActionTest.java | 1 + .../bzlmod/ModuleExtensionResolutionTest.java | 33 +++++ .../devtools/build/lib/cmdline/LabelTest.java | 6 + .../lib/cmdline/PackageIdentifierTest.java | 2 + .../build/lib/cmdline/RepositoryNameTest.java | 17 +++ .../StarlarkRuleClassFunctionsTest.java | 1 + .../util/BazelEvaluationTestCase.java | 1 + src/test/py/bazel/bzlmod/bazel_module_test.py | 135 ++++++++++++++++++ src/test/shell/bazel/BUILD | 1 + .../shell/bazel/bazel_layering_check_test.sh | 55 +++++++ 17 files changed, 329 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 6ea34327d30cf5..ab79b8dfdcf241 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -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 = diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java b/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java index 9bd7dd45b30ef7..d5671835122135 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java @@ -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(); @@ -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 loads, byte[] bzlTransitiveDigest) { return new AutoValue_BazelModuleContext( - label, repoMapping, filename, loads, bzlTransitiveDigest); + label, repoMapping, bestEffortMainRepoMapping, filename, loads, bzlTransitiveDigest); } public final Label.PackageContext packageContext() { diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index a2421a278eeb04..d0de7a6897453f 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -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 Label for use in BUILD files.

The exact form" + + " of the return value is explicitly unspecified and subject to change. The" + + " following properties are guaranteed for a Label l:

    " + + "
  • l.to_display_form() has no repository part if and only if" + + " l references the main repository;
  • " + + "
  • Label(l.to_display_form()) == l if the call to" + + " Label occurs in the main repository.
") + 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 diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java index a848ba5b560a6e..79a1f8c46c047f 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java @@ -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; /** @@ -215,7 +216,7 @@ public String getUnambiguousCanonicalForm() { *
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; } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java index d0b064512dd98d..984ba4ece2ac75 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java @@ -247,13 +247,15 @@ public String getCanonicalForm() { *
@protobuf *
if this repository is a WORKSPACE dependency and its name 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) *
@@protobuf~3.19.2 *
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(); } @@ -261,6 +263,9 @@ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) { // 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 diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 048d6c252f966a..aa87501f99b0d6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -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; @@ -765,6 +766,11 @@ private BzlLoadValue computeInternalWithCompiledBzl( if (repoMapping == null) { return null; } + Optional mainRepoMapping = + getMainRepositoryMapping(key, builtins.starlarkSemantics, env); + if (mainRepoMapping == null) { + return null; + } Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList