Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Label instances as keys in select #14447

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a nice PR!

My one quibble is documentation that says "A may be X or Y" without differentiating details is fundamentally ambiguous.

You explain the differentiation particularly well in macros.md. I wonder if a cross-reference there would help this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I created #14458 as a follow-up, could you take a look?

+ "<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,48 @@ 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"));
}
}