From 97f989d066fc87b86311c0ad4acff9571bf6de68 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 731a00f5a66a69..4001ad805f7b3e 100644 --- a/site/en/configure/attributes.md +++ b/site/en/configure/attributes.md @@ -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`, @@ -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} diff --git a/site/en/docs/configurable-attributes.md b/site/en/docs/configurable-attributes.md index 5b64ebca63990f..7e7eb9b619a831 100644 --- a/site/en/docs/configurable-attributes.md +++ b/site/en/docs/configurable-attributes.md @@ -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`, @@ -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} 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 a42be40ca39ebe..c4b8567e6a6cbd 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 @@ -690,7 +690,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 768d6a920e2eb5..456a0cddaaecde 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 @@ -173,7 +173,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. @@ -217,7 +220,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 @@ -225,9 +228,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 aa4d472fe2beec..c0df00d244e0c1 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.