ASP-based solver: model disjoint sets for multivalued variants (#22534)

* ASP-based solver: avoid adding values to variants when they're set

fixes #22533
fixes #21911

Added a rule that prevents any value to slip in a variant when the
variant is set explicitly. This is relevant for multi-valued variants,
in particular for those that have disjoint sets of values.

* Ensure disjoint sets have a clear semantics for external packages

4 files changed, 64 insertions, 2 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 03ab086423..d9ae6a170c 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -593,11 +593,16 @@ class SpackSolverSetup(object):
                 values = []
             elif isinstance(values, spack.variant.DisjointSetsOfValues):
                 union = set()
-                for s in values.sets:
+                # Encode the disjoint sets in the logic program
+                for sid, s in enumerate(values.sets):
+                    for value in s:
+                        self.gen.fact(fn.variant_value_from_disjoint_sets(
+                            pkg.name, name, value, sid
+                        ))
                     union.update(s)
                 values = union
 
-            # make sure that every variant has at least one posisble value
+            # make sure that every variant has at least one possible value
             if not values:
                 values = [variant.default]
 
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 0a73ba9eda..6ef9deef12 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -350,6 +350,15 @@ variant_set(Package, Variant) :- variant_set(Package, Variant, _).
 % A variant cannot have a value that is not also a possible value
 :- variant_value(Package, Variant, Value), not variant_possible_value(Package, Variant, Value).
 
+% Some multi valued variants accept multiple values from disjoint sets.
+% Ensure that we respect that constraint and we don't pick values from more
+% than one set at once
+:- variant_value(Package, Variant, Value1),
+   variant_value(Package, Variant, Value2),
+   variant_value_from_disjoint_sets(Package, Variant, Value1, Set1),
+   variant_value_from_disjoint_sets(Package, Variant, Value2, Set2),
+   Set1 != Set2.
+
 % variant_set is an explicitly set variant value. If it's not 'set',
 % we revert to the default value. If it is set, we force the set value
 variant_value(Package, Variant, Value)
@@ -409,6 +418,7 @@ variant_single_value(Package, "dev_path")
 #defined variant_possible_value/3.
 #defined variant_default_value_from_packages_yaml/3.
 #defined variant_default_value_from_package_py/3.
+#defined variant_value_from_disjoint_sets/4.
 
 %-----------------------------------------------------------------------------
 % Platform semantics
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index f53330fc7a..8636be75e7 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1138,3 +1138,35 @@ class TestConcretize(object):
         s = Spec(spec_str)
         with pytest.raises(RuntimeError, match='not found in package'):
             s.concretize()
+
+    @pytest.mark.regression('22533')
+    @pytest.mark.parametrize('spec_str,variant_name,expected_values', [
+        # Test the default value 'auto'
+        ('mvapich2', 'file_systems', ('auto',)),
+        # Test setting a single value from the disjoint set
+        ('mvapich2 file_systems=lustre', 'file_systems', ('lustre',)),
+        # Test setting multiple values from the disjoint set
+        ('mvapich2 file_systems=lustre,gpfs', 'file_systems',
+         ('lustre', 'gpfs')),
+    ])
+    def test_mv_variants_disjoint_sets_from_spec(
+            self, spec_str, variant_name, expected_values
+    ):
+        s = Spec(spec_str).concretized()
+        assert set(expected_values) == set(s.variants[variant_name].value)
+
+    @pytest.mark.regression('22533')
+    def test_mv_variants_disjoint_sets_from_packages_yaml(self):
+        external_mvapich2 = {
+            'mvapich2': {
+                'buildable': False,
+                'externals': [{
+                    'spec': 'mvapich2@2.3.1 file_systems=nfs,ufs',
+                    'prefix': '/usr'
+                }]
+            }
+        }
+        spack.config.set('packages', external_mvapich2)
+
+        s = Spec('mvapich2').concretized()
+        assert set(s.variants['file_systems'].value) == set(['ufs', 'nfs'])
diff --git a/var/spack/repos/builtin.mock/packages/mvapich2/package.py b/var/spack/repos/builtin.mock/packages/mvapich2/package.py
new file mode 100644
index 0000000000..dd62fc4c71
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/mvapich2/package.py
@@ -0,0 +1,15 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class Mvapich2(Package):
+    homepage = "http://www.homepage.org"
+    url = "http://www.someurl"
+
+    version('1.5', '9c5d5d4fe1e17dd12153f40bc5b6dbc0')
+
+    variant(
+        'file_systems',
+        description='List of the ROMIO file systems to activate',
+        values=auto_or_any_combination_of('lustre', 'gpfs', 'nfs', 'ufs'),
+    )