concretizer: try hard to obtain all needed variant_possible_value()'s (#20102)

Track all the variant values mentioned when emitting constraints, validate them
and emit a fact that allows them as possible values.

This modification ensures that open-ended variants (variants accepting any string 
or any integer) are projected to the finite set of values that are relevant for this 
concretization.

7 files changed, 87 insertions, 10 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 13bcfd2e91..2d4598bd32 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1104,7 +1104,14 @@ class SpackSolverSetup(object):
             if not isinstance(values, tuple):
                 values = (values,)
 
+            # perform validation of the variant and values
+            spec = spack.spec.Spec(pkg_name)
+            spec.update_variant_validate(variant_name, values)
+
             for value in values:
+                self.variant_values_from_specs.add(
+                    (pkg_name, variant.name, value)
+                )
                 self.gen.fact(fn.variant_default_value_from_packages_yaml(
                     pkg_name, variant.name, value
                 ))
@@ -1692,15 +1699,7 @@ class SpecBuilder(object):
             )
             return
 
-        pkg_class = spack.repo.path.get_pkg_class(pkg)
-
-        variant = self._specs[pkg].variants.get(name)
-        if variant:
-            # it's multi-valued
-            variant.append(value)
-        else:
-            variant = pkg_class.variants[name].make_variant(value)
-            self._specs[pkg].variants[name] = variant
+        self._specs[pkg].update_variant_validate(name, value)
 
     def version(self, pkg, version):
         self._specs[pkg].versions = ver([version])
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 96bf23a45c..728331d91c 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -183,6 +183,9 @@ external_spec(Package, ID) :-
 % if a variant is set to anything, it is considered 'set'.
 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).
+
 % 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)
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 46400859d3..88516470fb 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2893,6 +2893,40 @@ class Spec(object):
         if not_existing:
             raise vt.UnknownVariantError(spec, not_existing)
 
+    def update_variant_validate(self, variant_name, values):
+        """If it is not already there, adds the variant named
+        `variant_name` to the spec `spec` based on the definition
+        contained in the package metadata. Validates the variant and
+        values before returning.
+
+        Used to add values to a variant without being sensitive to the
+        variant being single or multi-valued. If the variant already
+        exists on the spec it is assumed to be multi-valued and the
+        values are appended.
+
+        Args:
+           variant_name: the name of the variant to add or append to
+           values: the value or values (as a tuple) to add/append
+                   to the variant
+        """
+        if not isinstance(values, tuple):
+            values = (values,)
+
+        pkg_variant = self.package_class.variants[variant_name]
+
+        for value in values:
+            if self.variants.get(variant_name):
+                msg = ("Cannot append a value to a single-valued "
+                       "variant with an already set value")
+                assert pkg_variant.multi, msg
+                self.variants[variant_name].append(value)
+            else:
+                variant = pkg_variant.make_variant(value)
+                self.variants[variant_name] = variant
+
+        pkg_variant.validate_or_raise(
+            self.variants[variant_name], self.package)
+
     def constrain(self, other, deps=True):
         """Merge the constraints of other with self.
 
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index f5f4ac4ad9..f98d9140fa 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -264,6 +264,10 @@ class TestConcretize(object):
         s.concretize()
         assert s['mpi'].version == ver('1.10.3')
 
+    def test_concretize_dependent_with_singlevalued_variant_type(self):
+        s = Spec('singlevalue-variant-dependent-type')
+        s.concretize()
+
     @pytest.mark.parametrize("spec,version", [
         ('dealii', 'develop'),
         ('xsdk', '0.4.0'),
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index 9ec304e624..9abac7221d 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -82,7 +82,9 @@ class TestConcretizePreferences(object):
          {'debug': True, 'opt': True, 'shared': False, 'static': False}),
         # Check a multivalued variant with multiple values set
         ('multivalue-variant', ['foo=bar,baz', 'fee=bar'],
-         {'foo': ('bar', 'baz'), 'fee': 'bar'})
+         {'foo': ('bar', 'baz'), 'fee': 'bar'}),
+        ('singlevalue-variant', ['fum=why'],
+         {'fum': 'why'})
     ])
     def test_preferred_variants(
             self, package_name, variant_value, expected_results
diff --git a/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent-type/package.py b/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent-type/package.py
new file mode 100644
index 0000000000..8b42a82b8d
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent-type/package.py
@@ -0,0 +1,16 @@
+# Copyright 2013-2020 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 SinglevalueVariantDependentType(Package):
+    """Simple package with one dependency that has a single-valued
+       variant with values=str"""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/archive-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    depends_on('singlevalue-variant fum=nope')
diff --git a/var/spack/repos/builtin.mock/packages/singlevalue-variant/package.py b/var/spack/repos/builtin.mock/packages/singlevalue-variant/package.py
new file mode 100644
index 0000000000..fa6eca1527
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/singlevalue-variant/package.py
@@ -0,0 +1,19 @@
+# Copyright 2013-2020 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 SinglevalueVariant(Package):
+    homepage = "http://www.llnl.gov"
+    url = "http://www.llnl.gov/mpileaks-1.0.tar.gz"
+
+    version(1.0, 'foobarbaz')
+
+    variant(
+        'fum',
+        description='Single-valued variant with type in values',
+        default='bar',
+        values=str,
+        multi=False
+    )