From b859571804f2095caaf018b172b59c90f185fd51 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 7 Feb 2023 11:22:14 -0800 Subject: [PATCH] Add `native.package_relative_label` function This essentially brings back the old `relative_to_caller_repository` param to the Label constructor, but with an arguably better API. It converts a label string into a Label object using the context of the calling package; this is useful for macro authors that want to convert string inputs into labels for reasons such as deduping. Also made `Label()` accept relative labels (like `:foo`) because there really isn't a good reason not to do so. Short design doc about the proposed API: https://docs.google.com/document/d/1_kMVWRHSBVkSsw1SLWPf3e-3RNSt55ZZYlzFzJkkwMo/edit Fixes https://github.com/bazelbuild/bazel/issues/17260 RELNOTES: Added a `native.package_relative_label()` function, which converts a label string to a Label object in the context of the calling package, in contrast to `Label()`, which does so in the context of the current .bzl file. Both functions now also accept relative labels such as `:foo`, and are idempotent. PiperOrigin-RevId: 507836895 Change-Id: Ic870fe564d96a77f05dd7258d32c031fca8cacb1 --- .../starlark/StarlarkRuleClassFunctions.java | 11 ++-- .../lib/packages/StarlarkNativeModule.java | 14 +++++ .../StarlarkNativeModuleApi.java | 33 +++++++++++ .../StarlarkRuleFunctionsApi.java | 24 +++++--- .../FakeStarlarkNativeModuleApi.java | 6 ++ .../FakeStarlarkRuleFunctionsApi.java | 9 ++- .../StarlarkRuleClassFunctionsTest.java | 7 +++ src/test/py/bazel/bzlmod/bazel_module_test.py | 57 +++++++++++++++++++ 8 files changed, 146 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 5d41477e082501..f436d0d62e964d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -997,8 +997,11 @@ public boolean isImmutable() { .build(); @Override - public Label label(String labelString, StarlarkThread thread) throws EvalException { - // The label string is interpeted with respect to the .bzl module containing the call to + public Label label(Object input, StarlarkThread thread) throws EvalException { + if (input instanceof Label) { + return (Label) input; + } + // The label string is interpreted with respect to the .bzl module containing the call to // `Label()`. An alternative to this approach that avoids stack inspection is to have each .bzl // module define its own copy of the `Label()` builtin embedding the module's own name. This // would lead to peculiarities like foo.bzl being able to call bar.bzl's `Label()` symbol to @@ -1007,9 +1010,9 @@ public Label label(String labelString, StarlarkThread thread) throws EvalExcepti BazelModuleContext moduleContext = BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)); try { - return Label.parseWithRepoContext(labelString, moduleContext.packageContext()); + return Label.parseWithPackageContext((String) input, moduleContext.packageContext()); } catch (LabelSyntaxException e) { - throw Starlark.errorf("Illegal absolute label syntax: %s", e.getMessage()); + throw Starlark.errorf("invalid label in Label(): %s", e.getMessage()); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index 5acf9f1f7ce931..34c4d51788470a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -621,6 +621,20 @@ public String repositoryName(StarlarkThread thread) throws EvalException { return packageId.getRepository().getNameWithAt(); } + @Override + public Label packageRelativeLabel(Object input, StarlarkThread thread) throws EvalException { + BazelStarlarkContext.from(thread).checkLoadingPhase("native.package_relative_label"); + if (input instanceof Label) { + return (Label) input; + } + try { + String s = (String) input; + return PackageFactory.getContext(thread).getBuilder().getLabelConverter().convert(s); + } catch (LabelSyntaxException e) { + throw Starlark.errorf("invalid label in native.package_relative_label: %s", e.getMessage()); + } + } + private static Dict getRuleDict(Rule rule, Mutability mu) throws EvalException { Dict.Builder values = Dict.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java index 7decf348785f5a..f61f0b993596e7 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.starlarkbuildapi; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.lib.cmdline.Label; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; import net.starlark.java.annot.StarlarkBuiltin; @@ -241,6 +242,38 @@ NoneType exportsFiles(Sequence srcs, Object visibility, Object licenses, Star useStarlarkThread = true) String repositoryName(StarlarkThread thread) throws EvalException; + @StarlarkMethod( + name = "package_relative_label", + doc = + "Converts the input string into a Label object, in the context of the" + + " package currently being initialized (that is, the BUILD file for" + + " which the current macro is executing). If the input is already a" + + " Label, it is returned unchanged.

This function may only be called" + + " while evaluating a BUILD file and the macros it directly or indirectly calls; it" + + " may not be called in (for instance) a rule implementation function.

The result" + + " of this function is the same Label value as would be produced by" + + " passing the given string to a label-valued attribute of a target declared in the" + + " BUILD file.

Usage note: The difference between this function and Label() is that Label() uses the" + + " context of the package of the .bzl file that called it, not the" + + " package of the BUILD file. Use Label() when you need to" + + " refer to a fixed target that is hardcoded into the macro, such as a compiler. Use" + + " package_relative_label() when you need to normalize a label string" + + " supplied by the BUILD file to a Label object. (There is no way to" + + " convert a string to a Label in the context of a package other than" + + " the BUILD file or the calling .bzl file. For that reason, outer macros should" + + " always prefer to pass Label objects to inner macros rather than label strings.)", + parameters = { + @Param( + name = "input", + allowedTypes = {@ParamType(type = String.class), @ParamType(type = Label.class)}, + doc = + "The input label string or Label object. If a Label object is passed, it's" + + " returned as is.") + }, + useStarlarkThread = true) + Label packageRelativeLabel(Object input, StarlarkThread thread) throws EvalException; + @StarlarkMethod( name = "subpackages", doc = diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java index 50e22aba8fecd5..0d4403490ebe46 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java @@ -701,18 +701,26 @@ StarlarkAspectApi aspect( @StarlarkMethod( name = "Label", doc = - "Creates a Label referring to a BUILD target. Use this function when you want to give a" - + " default value for the label attributes of a rule or when referring to a target" - + " via an absolute label from a macro. The argument must refer to an absolute label." - + " The repo part of the label (or its absence) is interpreted in the context of the" - + " repo where this Label() call appears. Example:
Label(\"//tools:default\")", + "Converts a label string into a Label object, in the context of the package" + + " where the calling .bzl source file lives. If the given value is" + + " already a Label, it is returned unchanged." + + "

For macros, a related function," + + " native.package_relative_label()," + + " converts the input into a Label in the context of the package" + + " currently being constructed. Use that function to mimic the string-to-label" + + " conversion that is automatically done by label-valued rule attributes.", parameters = { - @Param(name = "label_string", doc = "the label string."), + @Param( + name = "input", + allowedTypes = {@ParamType(type = String.class), @ParamType(type = Label.class)}, + doc = + "The input label string or Label object. If a Label object is passed, it's" + + " returned as is.") }, useStarlarkThread = true) @StarlarkConstructor - Label label(String labelString, StarlarkThread thread) throws EvalException; + Label label(Object input, StarlarkThread thread) throws EvalException; @StarlarkMethod( name = "exec_group", diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java index b4fd592582fc5f..e2e58aea15b662 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java @@ -16,6 +16,7 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkNativeModuleApi; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; @@ -77,6 +78,11 @@ public String repositoryName(StarlarkThread thread) { return ""; } + @Override + public Label packageRelativeLabel(Object input, StarlarkThread thread) throws EvalException { + return Label.parseCanonicalUnchecked("//:fake_label"); + } + @Override public Sequence subpackages( Sequence include, Sequence exclude, boolean allowEmpty, StarlarkThread thread) diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java index 6a2ecead06cd5f..f891b9db5f5c9a 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java @@ -171,11 +171,14 @@ public StarlarkCallable rule( } @Override - public Label label(String labelString, StarlarkThread thread) throws EvalException { + public Label label(Object input, StarlarkThread thread) throws EvalException { + if (input instanceof Label) { + return (Label) input; + } try { - return Label.parseCanonical(labelString); + return Label.parseCanonical((String) input); } catch (LabelSyntaxException e) { - throw Starlark.errorf("Illegal absolute label syntax: %s", labelString); + throw Starlark.errorf("Illegal absolute label syntax: %s", input); } } diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index b1c0fd10558c04..b179a1b053698f 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -1119,6 +1119,13 @@ public void testLabel() throws Exception { assertThat(result.toString()).isEqualTo("//foo/foo:foo"); } + @Test + public void testLabelIdempotence() throws Exception { + Object result = ev.eval("Label(Label('//foo/foo:foo'))"); + assertThat(result).isInstanceOf(Label.class); + assertThat(result.toString()).isEqualTo("//foo/foo:foo"); + } + @Test public void testLabelSameInstance() throws Exception { Object l1 = ev.eval("Label('//foo/foo:foo')"); diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index 57a0b9dec84a35..58c9d7f281b26d 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -884,5 +884,62 @@ def testJavaRunfilesLibraryRepoMapping(self): env_add={'RUNFILES_LIB_DEBUG': '1'}) self.AssertExitCode(exit_code, 0, stderr, stdout) + def testNativePackageRelativeLabel(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'module(name="foo")', + 'bazel_dep(name="bar")', + 'local_path_override(module_name="bar",path="bar")', + ], + ) + self.ScratchFile('WORKSPACE') + self.ScratchFile('BUILD') + self.ScratchFile( + 'defs.bzl', + [ + 'def mac(name):', + ' native.filegroup(name=name)', + ' print("1st: " + str(native.package_relative_label(":bleb")))', + ' print("2nd: " + str(native.package_relative_label(' + + '"//bleb:bleb")))', + ' print("3rd: " + str(native.package_relative_label(' + + '"@bleb//bleb:bleb")))', + ' print("4th: " + str(native.package_relative_label("//bleb")))', + ' print("5th: " + str(native.package_relative_label(' + + '"@@bleb//bleb:bleb")))', + ' print("6th: " + str(native.package_relative_label(Label(' + + '"//bleb"))))', + ], + ) + + self.ScratchFile( + 'bar/MODULE.bazel', + [ + 'module(name="bar")', + 'bazel_dep(name="foo", repo_name="bleb")', + ], + ) + self.ScratchFile('bar/WORKSPACE') + self.ScratchFile( + 'bar/quux/BUILD', + [ + 'load("@bleb//:defs.bzl", "mac")', + 'mac(name="book")', + ], + ) + + _, _, stderr = self.RunBazel( + ['build', '@bar//quux:book'], allow_failure=False + ) + stderr = '\n'.join(stderr) + self.assertIn('1st: @@bar~override//quux:bleb', stderr) + self.assertIn('2nd: @@bar~override//bleb:bleb', stderr) + self.assertIn('3rd: @@//bleb:bleb', stderr) + self.assertIn('4th: @@bar~override//bleb:bleb', stderr) + self.assertIn('5th: @@bleb//bleb:bleb', stderr) + self.assertIn('6th: @@//bleb:bleb', stderr) + + if __name__ == '__main__': unittest.main()