Skip to content

Commit

Permalink
Allow Label instances as keys in select
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum authored and copybara-github committed Dec 20, 2021
1 parent 78d0131 commit 69f8b17
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 11 deletions.
32 changes: 32 additions & 0 deletions site/docs/skylark/macros.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -84,9 +85,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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
+ "<a href=\"$BE_ROOT/../skylark/lib/Label.html\">Label</a> or a label string"
+ " that identifies a config_setting, constraint_setting, or constraint_value"
+ " instance."),
@Param(
name = "no_match_error",
defaultValue = "''",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: <br><pre"
"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: <br><pre"
+ " class=language-python>Label(\"//tools:default\")</pre>",
parameters = {
@Param(name = "label_string", doc = "the label string."),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -1569,4 +1571,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"));
}
}

0 comments on commit 69f8b17

Please sign in to comment.