Allow conditional variants (#24858)

A common question from users has been how to model variants 
that are new in new versions of a package, or variants that are 
dependent on other variants. Our stock answer so far has been
an unsatisfying combination of "just have it do nothing in the old
version" and "tell Spack it conflicts".

This PR enables conditional variants, on any spec condition. The 
syntax is straightforward, and matches that of previous features.

12 files changed, 193 insertions, 32 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index b9fbb18cf7..dbf1c83078 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -1419,6 +1419,60 @@ other similar operations:
             ).with_default('auto').with_non_feature_values('auto'),
         )
 
+^^^^^^^^^^^^^^^^^^^^
+Conditional Variants
+^^^^^^^^^^^^^^^^^^^^
+
+The variant directive accepts a ``when`` clause. The variant will only
+be present on specs that otherwise satisfy the spec listed as the
+``when`` clause. For example, the following class has a variant
+``bar`` when it is at version 2.0 or higher.
+
+.. code-block:: python
+
+   class Foo(Package):
+       ...
+       variant('bar', default=False, when='@2.0:', description='help message')
+
+The ``when`` clause follows the same syntax and accepts the same
+values as the ``when`` argument of
+:py:func:`spack.directives.depends_on`
+
+^^^^^^^^^^^^^^^^^^^
+Overriding Variants
+^^^^^^^^^^^^^^^^^^^
+
+Packages may override variants for several reasons, most often to
+change the default from a variant defined in a parent class or to
+change the conditions under which a variant is present on the spec.
+
+When a variant is defined multiple times, whether in the same package
+file or in a subclass and a superclass, the last definition is used
+for all attributes **except** for the ``when`` clauses. The ``when``
+clauses are accumulated through all invocations, and the variant is
+present on the spec if any of the accumulated conditions are
+satisfied.
+
+For example, consider the following package:
+
+.. code-block:: python
+
+   class Foo(Package):
+       ...
+       variant('bar', default=False, when='@1.0', description='help1')
+       variant('bar', default=True, when='platform=darwin', description='help2')
+       ...
+
+This package ``foo`` has a variant ``bar`` when the spec satisfies
+either ``@1.0`` or ``platform=darwin``, but not for other platforms at
+other versions. The default for this variant, when it is present, is
+always ``True``, regardless of which condition of the variant is
+satisfied. This allows packages to override variants in packages or
+build system classes from which they inherit, by modifying the variant
+values without modifying the ``when`` clause. It also allows a package
+to implement ``or`` semantics for a variant ``when`` clause by
+duplicating the variant definition.
+
 ------------------------------------
 Resources (expanding extra tarballs)
 ------------------------------------
diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index f19d5fa54b..e0c8cf2e03 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -521,7 +521,8 @@ To resolve this problem, please try the following:
 
         # Create a list of pairs. Each pair includes a configuration
         # option and whether or not that option is activated
-        if set(self.variants[variant].values) == set((True, False)):
+        variant_desc, _ = self.variants[variant]
+        if set(variant_desc.values) == set((True, False)):
             # BoolValuedVariant carry information about a single option.
             # Nonetheless, for uniformity of treatment we'll package them
             # in an iterable of one element.
@@ -534,8 +535,8 @@ To resolve this problem, please try the following:
             # package's build system. It excludes values which have special
             # meanings and do not correspond to features (e.g. "none")
             feature_values = getattr(
-                self.variants[variant].values, 'feature_values', None
-            ) or self.variants[variant].values
+                variant_desc.values, 'feature_values', None
+            ) or variant_desc.values
 
             options = [
                 (value,
diff --git a/lib/spack/spack/cmd/config.py b/lib/spack/spack/cmd/config.py
index 0dfb40c04e..7bdefd290e 100644
--- a/lib/spack/spack/cmd/config.py
+++ b/lib/spack/spack/cmd/config.py
@@ -433,7 +433,8 @@ def config_prefer_upstream(args):
                     or var_name not in spec.package.variants):
                 continue
 
-            if variant.value != spec.package.variants[var_name].default:
+            variant_desc, _ = spec.package.variants[var_name]
+            if variant.value != variant_desc.default:
                 variants.append(str(variant))
         variants.sort()
         variants = ' '.join(variants)
diff --git a/lib/spack/spack/cmd/info.py b/lib/spack/spack/cmd/info.py
index b4994a7b2d..73c632907a 100644
--- a/lib/spack/spack/cmd/info.py
+++ b/lib/spack/spack/cmd/info.py
@@ -57,7 +57,7 @@ def variant(s):
 class VariantFormatter(object):
     def __init__(self, variants):
         self.variants = variants
-        self.headers = ('Name [Default]', 'Allowed values', 'Description')
+        self.headers = ('Name [Default]', 'When', 'Allowed values', 'Description')
 
         # Formats
         fmt_name = '{0} [{1}]'
@@ -68,9 +68,11 @@ class VariantFormatter(object):
         self.column_widths = [len(x) for x in self.headers]
 
         # Expand columns based on max line lengths
-        for k, v in variants.items():
+        for k, e in variants.items():
+            v, w = e
             candidate_max_widths = (
                 len(fmt_name.format(k, self.default(v))),  # Name [Default]
+                len(str(w)),
                 len(v.allowed_values),  # Allowed values
                 len(v.description)  # Description
             )
@@ -78,26 +80,29 @@ class VariantFormatter(object):
             self.column_widths = (
                 max(self.column_widths[0], candidate_max_widths[0]),
                 max(self.column_widths[1], candidate_max_widths[1]),
-                max(self.column_widths[2], candidate_max_widths[2])
+                max(self.column_widths[2], candidate_max_widths[2]),
+                max(self.column_widths[3], candidate_max_widths[3])
             )
 
         # Don't let name or possible values be less than max widths
         _, cols = tty.terminal_size()
         max_name = min(self.column_widths[0], 30)
-        max_vals = min(self.column_widths[1], 20)
+        max_when = min(self.column_widths[1], 30)
+        max_vals = min(self.column_widths[2], 20)
 
         # allow the description column to extend as wide as the terminal.
         max_description = min(
-            self.column_widths[2],
+            self.column_widths[3],
             # min width 70 cols, 14 cols of margins and column spacing
             max(cols, 70) - max_name - max_vals - 14,
         )
-        self.column_widths = (max_name, max_vals, max_description)
+        self.column_widths = (max_name, max_when, max_vals, max_description)
 
         # Compute the format
-        self.fmt = "%%-%ss%%-%ss%%s" % (
+        self.fmt = "%%-%ss%%-%ss%%-%ss%%s" % (
             self.column_widths[0] + 4,
-            self.column_widths[1] + 4
+            self.column_widths[1] + 4,
+            self.column_widths[2] + 4
         )
 
     def default(self, v):
@@ -115,21 +120,27 @@ class VariantFormatter(object):
             underline = tuple([w * "=" for w in self.column_widths])
             yield '    ' + self.fmt % underline
             yield ''
-            for k, v in sorted(self.variants.items()):
+            for k, e in sorted(self.variants.items()):
+                v, w = e
                 name = textwrap.wrap(
                     '{0} [{1}]'.format(k, self.default(v)),
                     width=self.column_widths[0]
                 )
+                if len(w) == 1:
+                    w = w[0]
+                    if w == spack.spec.Spec():
+                        w = '--'
+                when = textwrap.wrap(str(w), width=self.column_widths[1])
                 allowed = v.allowed_values.replace('True, False', 'on, off')
-                allowed = textwrap.wrap(allowed, width=self.column_widths[1])
+                allowed = textwrap.wrap(allowed, width=self.column_widths[2])
                 description = []
                 for d_line in v.description.split('\n'):
                     description += textwrap.wrap(
                         d_line,
-                        width=self.column_widths[2]
+                        width=self.column_widths[3]
                     )
                 for t in zip_longest(
-                        name, allowed, description, fillvalue=''
+                        name, when, allowed, description, fillvalue=''
                 ):
                     yield "    " + self.fmt % t
 
@@ -232,7 +243,7 @@ def print_text_info(pkg):
 
     formatter = VariantFormatter(pkg.variants)
     for line in formatter.lines:
-        color.cprint(line)
+        color.cprint(color.cescape(line))
 
     if hasattr(pkg, 'phases') and pkg.phases:
         color.cprint('')
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 1b865c32cc..aed8fe7713 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -384,7 +384,8 @@ class Concretizer(object):
         changed = False
         preferred_variants = PackagePrefs.preferred_variants(spec.name)
         pkg_cls = spec.package_class
-        for name, variant in pkg_cls.variants.items():
+        for name, entry in pkg_cls.variants.items():
+            variant, when = entry
             var = spec.variants.get(name, None)
             if var and '*' in var:
                 # remove variant wildcard before concretizing
@@ -392,12 +393,16 @@ class Concretizer(object):
                 # multivalue variant, a concrete variant cannot have the value
                 # wildcard, and a wildcard does not constrain a variant
                 spec.variants.pop(name)
-            if name not in spec.variants:
+            if name not in spec.variants and any(spec.satisfies(w)
+                                                 for w in when):
                 changed = True
                 if name in preferred_variants:
                     spec.variants[name] = preferred_variants.get(name)
                 else:
                     spec.variants[name] = variant.make_default()
+            if name in spec.variants and not any(spec.satisfies(w)
+                                                 for w in when):
+                raise vt.InvalidVariantForSpecError(name, when, spec)
 
         return changed
 
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 863001a601..f7400f4da9 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -244,7 +244,7 @@ class DirectiveMeta(type):
                 if DirectiveMeta._when_constraints_from_context:
                     # Check that directives not yet supporting the when= argument
                     # are not used inside the context manager
-                    if decorated_function.__name__ in ('version', 'variant'):
+                    if decorated_function.__name__ == 'version':
                         msg = ('directive "{0}" cannot be used within a "when"'
                                ' context since it does not support a "when=" '
                                'argument')
@@ -562,7 +562,8 @@ def variant(
         description='',
         values=None,
         multi=None,
-        validator=None):
+        validator=None,
+        when=None):
     """Define a variant for the package. Packager can specify a default
     value as well as a text description.
 
@@ -581,6 +582,8 @@ def variant(
             logic. It receives the package name, the variant name and a tuple
             of values and should raise an instance of SpackError if the group
             doesn't meet the additional constraints
+        when (spack.spec.Spec, bool): optional condition on which the
+            variant applies
 
     Raises:
         DirectiveError: if arguments passed to the directive are invalid
@@ -640,14 +643,23 @@ def variant(
     description = str(description).strip()
 
     def _execute_variant(pkg):
+        when_spec = make_when_spec(when)
+        when_specs = [when_spec]
+
         if not re.match(spack.spec.identifier_re, name):
             directive = 'variant'
             msg = "Invalid variant name in {0}: '{1}'"
             raise DirectiveError(directive, msg.format(pkg.name, name))
 
-        pkg.variants[name] = spack.variant.Variant(
+        if name in pkg.variants:
+            # We accumulate when specs, but replace the rest of the variant
+            # with the newer values
+            _, orig_when = pkg.variants[name]
+            when_specs += orig_when
+
+        pkg.variants[name] = (spack.variant.Variant(
             name, default, description, values, multi, validator
-        )
+        ), when_specs)
     return _execute_variant
 
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 2d70df210f..e83b38cf8d 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -721,8 +721,12 @@ class SpackSolverSetup(object):
         self.gen.newline()
 
         # variants
-        for name, variant in sorted(pkg.variants.items()):
-            self.gen.fact(fn.variant(pkg.name, name))
+        for name, entry in sorted(pkg.variants.items()):
+            variant, when = entry
+
+            for w in when:
+                cond_id = self.condition(w, name=pkg.name)
+                self.gen.fact(fn.variant_condition(cond_id, pkg.name, name))
 
             single_value = not variant.multi
             if single_value:
@@ -788,7 +792,7 @@ class SpackSolverSetup(object):
 
         Arguments:
             required_spec (spack.spec.Spec): the spec that triggers this condition
-            imposed_spec (spack.spec.Spec or None): the sepc with constraints that
+            imposed_spec (spack.spec.Spec or None): the spec with constraints that
                 are imposed when this condition is triggered
             name (str or None): name for `required_spec` (required if
                 required_spec is anonymous, ignored if not)
@@ -1087,7 +1091,7 @@ class SpackSolverSetup(object):
                     reserved_names = spack.directives.reserved_names
                     if not spec.virtual and vname not in reserved_names:
                         try:
-                            variant_def = spec.package.variants[vname]
+                            variant_def, _ = spec.package.variants[vname]
                         except KeyError:
                             msg = 'variant "{0}" not found in package "{1}"'
                             raise RuntimeError(msg.format(vname, spec.name))
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index a65a0d530e..00c8abe82d 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -350,6 +350,20 @@ external_conditions_hold(Package, LocalIndex) :-
 %-----------------------------------------------------------------------------
 % Variant semantics
 %-----------------------------------------------------------------------------
+% a variant is a variant of a package if it is a variant under some condition
+% and that condition holds
+variant(Package, Variant) :- variant_condition(ID, Package, Variant),
+                             condition_holds(ID).
+
+% a variant cannot be set if it is not a variant on the package
+:- variant_set(Package, Variant),
+   not variant(Package, Variant),
+   error("Unsatisfied conditional variants cannot be set").
+
+% a variant cannot take on a value if it is not a variant of the package
+:- variant_value(Package, Variant, _), not variant(Package, Variant),
+   error("Unsatisfied conditional variants cannot take on a variant value").
+
 % one variant value for single-valued variants.
 1 {
   variant_value(Package, Variant, Value)
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 2c5e6086e1..ca3ad5f317 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -3081,7 +3081,7 @@ class Spec(object):
         if not isinstance(values, tuple):
             values = (values,)
 
-        pkg_variant = self.package_class.variants[variant_name]
+        pkg_variant, _ = self.package_class.variants[variant_name]
 
         for value in values:
             if self.variants.get(variant_name):
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index b2ac1b6446..62d2af8bd3 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -16,6 +16,7 @@ import spack.concretize
 import spack.error
 import spack.platforms
 import spack.repo
+import spack.variant as vt
 from spack.concretize import find_spec
 from spack.spec import Spec
 from spack.util.mock_package import MockPackageMultiRepo
@@ -739,6 +740,41 @@ class TestConcretize(object):
         assert s.satisfies(expected_str)
 
     @pytest.mark.parametrize('spec_str,expected,unexpected', [
+        ('conditional-variant-pkg@1.0',
+         ['two_whens'],
+         ['version_based', 'variant_based']),
+        ('conditional-variant-pkg@2.0',
+         ['version_based', 'variant_based'],
+         ['two_whens']),
+        ('conditional-variant-pkg@2.0~version_based',
+         ['version_based'],
+         ['variant_based', 'two_whens']),
+        ('conditional-variant-pkg@2.0+version_based+variant_based',
+         ['version_based', 'variant_based', 'two_whens'],
+         [])
+    ])
+    def test_conditional_variants(self, spec_str, expected, unexpected):
+        s = Spec(spec_str).concretized()
+
+        for var in expected:
+            assert s.satisfies('%s=*' % var)
+        for var in unexpected:
+            assert not s.satisfies('%s=*' % var)
+
+    @pytest.mark.parametrize('bad_spec', [
+        '@1.0~version_based',
+        '@1.0+version_based',
+        '@2.0~version_based+variant_based',
+        '@2.0+version_based~variant_based+two_whens',
+    ])
+    def test_conditional_variants_fail(self, bad_spec):
+        with pytest.raises(
+                (spack.error.UnsatisfiableSpecError,
+                 vt.InvalidVariantForSpecError)
+        ):
+            _ = Spec('conditional-variant-pkg' + bad_spec).concretized()
+
+    @pytest.mark.parametrize('spec_str,expected,unexpected', [
         ('py-extension3 ^python@3.5.1', [], ['py-extension1']),
         ('py-extension3 ^python@2.7.11', ['py-extension1'], []),
         ('py-extension3@1.0 ^python@2.7.11', ['patchelf@0.9'], []),
diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index f65e008ef4..35840d85da 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -247,7 +247,8 @@ def test_variant_defaults_are_parsable_from_cli():
     """Ensures that variant defaults are parsable from cli."""
     failing = []
     for pkg in spack.repo.path.all_packages():
-        for variant_name, variant in pkg.variants.items():
+        for variant_name, entry in pkg.variants.items():
+            variant, _ = entry
             default_is_parsable = (
                 # Permitting a default that is an instance on 'int' permits
                 # to have foo=false or foo=0. Other falsish values are
@@ -262,7 +263,8 @@ def test_variant_defaults_are_parsable_from_cli():
 def test_variant_defaults_listed_explicitly_in_values():
     failing = []
     for pkg in spack.repo.path.all_packages():
-        for variant_name, variant in pkg.variants.items():
+        for variant_name, entry in pkg.variants.items():
+            variant, _ = entry
             vspec = variant.make_default()
             try:
                 variant.validate_or_raise(vspec, pkg=pkg)
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
index 3badd30c20..7550af95fa 100644
--- a/lib/spack/spack/variant.py
+++ b/lib/spack/spack/variant.py
@@ -181,6 +181,17 @@ class Variant(object):
             return BoolValuedVariant
         return SingleValuedVariant
 
+    def __eq__(self, other):
+        return (self.name == other.name and
+                self.default == other.default and
+                self.values == other.values and
+                self.multi == other.multi and
+                self.single_value_validator == other.single_value_validator and
+                self.group_validator == other.group_validator)
+
+    def __ne__(self, other):
+        return not self == other
+
 
 def implicit_variant_conversion(method):
     """Converts other to type(self) and calls method(self, other)
@@ -645,10 +656,10 @@ def substitute_abstract_variants(spec):
                 new_variant = SingleValuedVariant(name, v._original_value)
                 spec.variants.substitute(new_variant)
             continue
-        pkg_variant = spec.package_class.variants.get(name, None)
-        if not pkg_variant:
+        if name not in spec.package_class.variants:
             failed.append(name)
             continue
+        pkg_variant, _ = spec.package_class.variants[name]
         new_variant = pkg_variant.make_variant(v._original_value)
         pkg_variant.validate_or_raise(new_variant, spec.package_class)
         spec.variants.substitute(new_variant)
@@ -880,6 +891,16 @@ class InvalidVariantValueError(error.SpecError):
         )
 
 
+class InvalidVariantForSpecError(error.SpecError):
+    """Raised when an invalid conditional variant is specified."""
+    def __init__(self, variant, when, spec):
+        msg = "Invalid variant {0} for spec {1}.\n"
+        msg += "{0} is only available for {1.name} when satisfying one of {2}."
+        super(InvalidVariantForSpecError, self).__init__(
+            msg.format(variant, spec, when)
+        )
+
+
 class UnsatisfiableVariantSpecError(error.UnsatisfiableSpecError):
     """Raised when a spec variant conflicts with package constraints."""