From 1474b5b5b41dfb680674e37975b6e1754c3a7445 Mon Sep 17 00:00:00 2001 From: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Date: Thu, 13 Apr 2023 11:56:08 -0700 Subject: [PATCH] Allow multiple matching select branches if they resolve to the same value (#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 --- 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.