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

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
  • Loading branch information
AlessandroPatti authored and copybara-github committed Apr 12, 2023
1 parent 5ab5d80 commit d43737f
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 @@ -73,11 +73,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 @@ -518,7 +520,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 @@ -73,11 +73,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 @@ -518,7 +520,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 @@ -690,7 +690,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 @@ -173,7 +173,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 @@ -217,17 +220,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 d43737f

Please sign in to comment.