From 1683a5c8cdf21857f1b3f8563ac30f8594155bd4 Mon Sep 17 00:00:00 2001 From: Alessandro Patti Date: Tue, 11 Apr 2023 21:31:54 -0700 Subject: [PATCH] Allow multiple matching select branches if they resolve to the same value 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 --- site/en/configure/attributes.md | 14 ++++---- site/en/docs/configurable-attributes.md | 14 ++++---- .../build/docgen/templates/be/functions.vm | 2 +- .../packages/ConfiguredAttributeMapper.java | 15 +++++--- .../analysis/ConfigurableAttributesTest.java | 34 ++++++++++++++++++- 5 files changed, 60 insertions(+), 19 deletions(-) diff --git a/site/en/configure/attributes.md b/site/en/configure/attributes.md index 3e30140bce51a0..25961f9546dc61 100644 --- a/site/en/configure/attributes.md +++ b/site/en/configure/attributes.md @@ -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`, @@ -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} diff --git a/site/en/docs/configurable-attributes.md b/site/en/docs/configurable-attributes.md index 3e30140bce51a0..25961f9546dc61 100644 --- a/site/en/docs/configurable-attributes.md +++ b/site/en/docs/configurable-attributes.md @@ -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`, @@ -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} diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm index 0c976a2c16b1fd..dc900dc214b95c 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm @@ -672,7 +672,7 @@ sh_binary( demonstrated in Example 2 below.
  • 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.
  • The special pseudo-label //conditions:default is considered to match if no other condition matches. If this condition diff --git a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java index 1feff01fcff809..631910d7a904be 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java +++ b/src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java @@ -176,7 +176,10 @@ private static class ConfigKeyAndValue { private ConfigKeyAndValue resolveSelector(String attributeName, Selector selector) throws ValidationException { - Map> matchingConditions = new LinkedHashMap<>(); + // Use a LinkedHashMap to guarantee a deterministic branch selection when multiple branches + // matches but they + // resolve to the same value. + LinkedHashMap> 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. @@ -220,7 +223,7 @@ private ConfigKeyAndValue 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 @@ -228,9 +231,11 @@ private ConfigKeyAndValue resolveSelector(String attributeName, Selector< + 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. 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 4df31b59249333..12cceb731ca8bc 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 @@ -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."); } /** @@ -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.