From 60f757c0831f9fbb2415fb0105f964201faa9fa0 Mon Sep 17 00:00:00 2001 From: Brentley Jones Date: Wed, 9 Feb 2022 07:24:36 -0600 Subject: [PATCH] Allow Label instances as keys in select (#14755) When a macro specifies a label string as a key in a select, this label is resolved relative to the site of use rather than the .bzl file the macro is defined in. The resolution will lead to incorrect results if the repository that uses the macro has a different repo mapping, e.g. because it is created by another Bazel module. This can be solved by allowing macros to specify label instances created with the `Label` constructor instead of label strings everywhere, which previously was not possible in select. This commit also updates the docs for Label, select and macros. Fixes #14259. Closes #14447. PiperOrigin-RevId: 417386977 (cherry picked from commit 69f8b172ae8495587751564284fc126300cae6fe) Co-authored-by: Fabian Meumertzheim --- site/docs/skylark/macros.md | 32 +++++++++++ .../build/lib/packages/SelectorList.java | 5 +- .../build/lib/packages/StarlarkLibrary.java | 6 ++- .../StarlarkRuleFunctionsApi.java | 9 ++-- .../analysis/ConfigurableAttributesTest.java | 53 +++++++++++++++++-- 5 files changed, 94 insertions(+), 11 deletions(-) diff --git a/site/docs/skylark/macros.md b/site/docs/skylark/macros.md index e879106bd44500..85048a5cafe111 100644 --- a/site/docs/skylark/macros.md +++ b/site/docs/skylark/macros.md @@ -143,6 +143,38 @@ macro), use the function [native.package_name()](lib/native.html#package_name). Note that `native` can only be used in `.bzl` files, and not in `WORKSPACE` or `BUILD` files. +## Label resolution in macros + +Since macros are evaluated in the [loading phase](concepts.md#evaluation-model), +label strings such as `"//foo:bar"` that occur in a macro are interpreted +relative to the `BUILD` file in which the macro is used rather than relative to +the `.bzl` file in which it is defined. This behavior is generally undesirable +for macros that are meant to be used in other repositories, e.g. because they +are part of a published Starlark ruleset. + +To get the same behavior as for Starlark rules, wrap the label strings with the +[`Label`](lib/Label.html#Label) constructor: + +```python +# @my_ruleset//rules:defs.bzl +def my_cc_wrapper(name, deps = [], **kwargs): + native.cc_library( + name = name, + deps = deps + select({ + # Due to the use of Label, this label is resolved within @my_ruleset, + # regardless of its site of use. + Label("//config:needs_foo"): [ + # Due to the use of Label, this label will resolve to the correct target + # even if the canonical name of @dep_of_my_ruleset should be different + # in the main workspace, e.g. due to repo mappings. + Label("@dep_of_my_ruleset//tools:foo"), + ], + "//conditions:default": [], + }), + **kwargs, + ) +``` + ## Debugging * `bazel query --output=build //my/path:all` will show you how the `BUILD` file diff --git a/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java b/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java index 799b05023f8fcb..4a2547dbaf3e6a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SelectorList.java @@ -16,6 +16,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import java.util.Arrays; @@ -87,9 +88,9 @@ public static Object select(Dict dict, String noMatchError) throws EvalExc + " to match"); } for (Object key : dict.keySet()) { - if (!(key instanceof String)) { + if (!(key instanceof String || key instanceof Label)) { throw Starlark.errorf( - "select: got %s for dict key, want a label string", Starlark.type(key)); + "select: got %s for dict key, want a Label or label string", Starlark.type(key)); } } return SelectorList.of(new SelectorValue(dict, noMatchError)); diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkLibrary.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkLibrary.java index 75bf5538a31545..6c9025e81923b9 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkLibrary.java @@ -330,8 +330,10 @@ public Depset depset( name = "x", positional = true, doc = - "A dict that maps configuration conditions to values. Each key is a label string" - + " that identifies a config_setting instance."), + "A dict that maps configuration conditions to values. Each key is a " + + "Label or a label string" + + " that identifies a config_setting, constraint_setting, or constraint_value" + + " instance."), @Param( name = "no_match_error", defaultValue = "''", 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 5af218fe924b36..7365caa4ac62a8 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 @@ -579,10 +579,11 @@ StarlarkAspectApi aspect( @StarlarkMethod( name = "Label", doc = - "Creates a Label referring to a BUILD target. Use this function only when you want to" - + " give a default value for the label attributes. 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\")", parameters = { @Param(name = "label_string", doc = "the label string."), diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java index 79fb42bbc258af..fb57ba1e020bbb 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java @@ -427,7 +427,8 @@ public void configKeyTypeChecking_Int() throws Exception { " name = 'int_key',", " srcs = select({123: ['a.java']})", ")"); - assertTargetError("//java/foo:int_key", "select: got int for dict key, want a label string"); + assertTargetError( + "//java/foo:int_key", "select: got int for dict key, want a Label or label string"); } @Test @@ -439,7 +440,8 @@ public void configKeyTypeChecking_Bool() throws Exception { " name = 'bool_key',", " srcs = select({True: ['a.java']})", ")"); - assertTargetError("//java/foo:bool_key", "select: got bool for dict key, want a label string"); + assertTargetError( + "//java/foo:bool_key", "select: got bool for dict key, want a Label or label string"); } @Test @@ -452,7 +454,7 @@ public void configKeyTypeChecking_None() throws Exception { " srcs = select({None: ['a.java']})", ")"); assertTargetError( - "//java/foo:none_key", "select: got NoneType for dict key, want a label string"); + "//java/foo:none_key", "select: got NoneType for dict key, want a Label or label string"); } @Test @@ -1526,4 +1528,49 @@ public void publicVisibilityConfigSetting_defaultIsPrivate() throws Exception { assertThat(getConfiguredTarget("//a:binary")).isNotNull(); assertNoEvents(); } + + @Test + public void selectWithLabelKeysInMacro() throws Exception { + writeConfigRules(); + scratch.file("java/BUILD"); + scratch.file( + "java/macros.bzl", + "def my_java_binary(name, deps = [], **kwargs):", + " native.java_binary(", + " name = name,", + " deps = select({", + " Label('//conditions:a'): [Label('//java/foo:a')],", + " '//conditions:b': [Label('//java/foo:b')],", + " }) + select({", + " '//conditions:a': [Label('//java/foo:a2')],", + " Label('//conditions:b'): [Label('//java/foo:b2')],", + " }),", + " **kwargs,", + " )"); + scratch.file( + "java/foo/BUILD", + "load('//java:macros.bzl', 'my_java_binary')", + "my_java_binary(", + " name = 'binary',", + " srcs = ['binary.java'],", + ")", + "java_library(", + " name = 'a',", + " srcs = ['a.java'])", + "java_library(", + " name = 'b',", + " srcs = ['b.java'])", + "java_library(", + " name = 'a2',", + " srcs = ['a2.java'])", + "java_library(", + " name = 'b2',", + " srcs = ['b2.java'])"); + + checkRule( + "//java/foo:binary", + "--foo=b", + /*expected:*/ ImmutableList.of("bin java/foo/libb.jar", "bin java/foo/libb2.jar"), + /*not expected:*/ ImmutableList.of("bin java/foo/liba.jar", "bin java/foo/liba2.jar")); + } }