Skip to content

Commit

Permalink
Allow multiple matching select branches if they resolve to the same v…
Browse files Browse the repository at this point in the history
…alue (#18066)

Select branches that map to the same value can unambiguosly be resolved even when multiple are true. This is currently not allowed and requires the use of constructs like `bazel-skylib`'s `selects.config_setting_group`. With this change, assuming A and B are true, the following is allowed:
```
select({
     "//:A": "Value",
     "//:B": "Value",
})
```

Closes #14874.

PiperOrigin-RevId: 523597091
Change-Id: I751809b1b9e11d20a86ae2af4bbdb0228fd3c670

Co-authored-by: Alessandro Patti <ale812@yahoo.it>
  • Loading branch information
ShreeM01 and AlessandroPatti authored Apr 13, 2023
1 parent b9e6f7d commit 1474b5b
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 19 deletions.
14 changes: 8 additions & 6 deletions site/en/configure/attributes.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ command line. Specifically, `deps` becomes:
targets. By using `select()` in a configurable attribute, the attribute
effectively adopts different values when different conditions hold.

Matches must be unambiguous: either exactly one condition must match or, if
multiple conditions match, one's `values` must be a strict superset of all
others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}` is an
unambiguous specialization of `values = {"cpu": "x86"}`. The built-in condition
[`//conditions:default`](#default-condition) automatically matches when
Matches must be unambiguous: if multiple conditions match then either
* They all resolve to the same value. For example, when running on linux x86, this is unambiguous
`{"@platforms//os:linux": "Hello", "@platforms//cpu:x86_64": "Hello"}` because both branches resolve to "hello".
* One's `values` is a strict superset of all others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}`
is an unambiguous specialization of `values = {"cpu": "x86"}`.

The built-in condition [`//conditions:default`](#default-condition) automatically matches when
nothing else does.

While this example uses `deps`, `select()` works just as well on `srcs`,
Expand Down Expand Up @@ -516,7 +518,7 @@ Unlike `selects.with_or`, different targets can share `:config1_or_2` across
different attributes.
It's an error for multiple conditions to match unless one is an unambiguous
"specialization" of the others. See [here](#configurable-build-example) for details.
"specialization" of the others or they all resolve to the same value. See [here](#configurable-build-example) for details.

## AND chaining {:#and-chaining}

Expand Down
14 changes: 8 additions & 6 deletions site/en/docs/configurable-attributes.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ command line. Specifically, `deps` becomes:
targets. By using `select()` in a configurable attribute, the attribute
effectively adopts different values when different conditions hold.

Matches must be unambiguous: either exactly one condition must match or, if
multiple conditions match, one's `values` must be a strict superset of all
others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}` is an
unambiguous specialization of `values = {"cpu": "x86"}`. The built-in condition
[`//conditions:default`](#default-condition) automatically matches when
Matches must be unambiguous: if multiple conditions match then either
* They all resolve to the same value. For example, when running on linux x86, this is unambiguous
`{"@platforms//os:linux": "Hello", "@platforms//cpu:x86_64": "Hello"}` because both branches resolve to "hello".
* One's `values` is a strict superset of all others'. For example, `values = {"cpu": "x86", "compilation_mode": "dbg"}`
is an unambiguous specialization of `values = {"cpu": "x86"}`.

The built-in condition [`//conditions:default`](#default-condition) automatically matches when
nothing else does.

While this example uses `deps`, `select()` works just as well on `srcs`,
Expand Down Expand Up @@ -516,7 +518,7 @@ Unlike `selects.with_or`, different targets can share `:config1_or_2` across
different attributes.
It's an error for multiple conditions to match unless one is an unambiguous
"specialization" of the others. See [here](#configurable-build-example) for details.
"specialization" of the others or they all resolve to the same value. See [here](#configurable-build-example) for details.

## AND chaining {:#and-chaining}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ sh_binary(
demonstrated in Example 2 below.
</li>
<li>If multiple conditions match and one is not a specialization of all the
others, Bazel fails with an error.
others, Bazel fails with an error, unless all conditions resolve to the same value.
</li>
<li>The special pseudo-label <code>//conditions:default</code> is
considered to match if no other condition matches. If this condition
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,10 @@ private static class ConfigKeyAndValue<T> {

private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<T> selector)
throws ValidationException {
Map<Label, ConfigKeyAndValue<T>> matchingConditions = new LinkedHashMap<>();
// Use a LinkedHashMap to guarantee a deterministic branch selection when multiple branches
// matches but they
// resolve to the same value.
LinkedHashMap<Label, ConfigKeyAndValue<T>> matchingConditions = new LinkedHashMap<>();
// Use a LinkedHashSet to guarantee deterministic error message ordering. We use a LinkedHashSet
// vs. a more general SortedSet because the latter supports insertion-order, which should more
// closely match how users see select() structures in BUILD files.
Expand Down Expand Up @@ -220,17 +223,19 @@ private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<
}
}

if (matchingConditions.size() > 1) {
if (matchingConditions.values().stream().map(s -> s.value).distinct().count() > 1) {
throw new ValidationException(
"Illegal ambiguous match on configurable attribute \""
+ attributeName
+ "\" in "
+ getLabel()
+ ":\n"
+ Joiner.on("\n").join(matchingConditions.keySet())
+ "\nMultiple matches are not allowed unless one is unambiguously more specialized.");
} else if (matchingConditions.size() == 1) {
return Iterables.getOnlyElement(matchingConditions.values());
+ "\nMultiple matches are not allowed unless one is unambiguously "
+ "more specialized or they resolve to the same value. "
+ "See https://bazel.build/reference/be/functions#select.");
} else if (matchingConditions.size() > 0) {
return Iterables.getFirst(matchingConditions.values(), null);
}

// If nothing matched, choose the default condition.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,8 @@ public void multipleMatches() throws Exception {
"Illegal ambiguous match on configurable attribute \"srcs\" in //a:gen:\n"
+ "//conditions:dup1\n"
+ "//conditions:dup2\n"
+ "Multiple matches are not allowed unless one is unambiguously more specialized.");
+ "Multiple matches are not allowed unless one is unambiguously more specialized "
+ "or they resolve to the same value.");
}

/**
Expand Down Expand Up @@ -688,6 +689,37 @@ public void multipleMatchesConditionAndSubcondition() throws Exception {
"bin java/a/libgeneric.jar", "bin java/a/libprecise.jar"));
}

/** Tests that multiple matches are allowed for conditions where the value is the same. */
@Test
public void multipleMatchesSameValue() throws Exception {
reporter.removeHandler(failFastHandler); // Expect errors.
scratch.file(
"conditions/BUILD",
"config_setting(",
" name = 'dup1',",
" values = {'compilation_mode': 'opt'})",
"config_setting(",
" name = 'dup2',",
" values = {'define': 'foo=bar'})");
scratch.file(
"a/BUILD",
"genrule(",
" name = 'gen',",
" cmd = '',",
" outs = ['gen.out'],",
" srcs = select({",
" '//conditions:dup1': ['a.in'],",
" '//conditions:dup2': ['a.in'],",
" '" + BuildType.Selector.DEFAULT_CONDITION_KEY + "': [':default.in'],",
" }))");
checkRule(
"//a:gen",
"srcs",
ImmutableList.of("-c", "opt", "--define", "foo=bar"),
/*expected:*/ ImmutableList.of("src a/a.in"),
/*not expected:*/ ImmutableList.of("src a/default.in"));
}

/**
* Tests that when multiple conditions match but one condition is more specialized than the
* others, it is chosen and there is no error.
Expand Down

0 comments on commit 1474b5b

Please sign in to comment.