From c4521535e718ec9c0367b3825938dc06f1bc8aad Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Sat, 5 Jan 2019 04:02:34 +0100 Subject: [PATCH] Multi-valued variants: better support for combinations (#9481) This enforces conventions that allow for correct handling of multi-valued variants where specifying no value is an option, and adds convenience functionality for specifying multi-valued variants with conflicting sets of values. This also adds a notion of "feature values" for variants, which are those that are understood by the build system (e.g. those that would appear as configure options). In more detail: * Add documentation on variants to the packaging guide * Forbid usage of '' or None as a possible variant value, in particular as a default. To indicate choosing no value, the user must explicitly define an option like 'none'. Without this, multi-valued variants with default set to None were not parsable from the command line (Fixes #6314) * Add "disjoint_sets" function to support the declaration of multi-valued variants with conflicting sets of options. For example a variant "foo" with possible values "a", "b", and "c" where "c" is exclusive of the other values ("foo=a,b" and "foo=c" are valid but "foo=a,c" is not). * Add "any_combination_of" function to support the declaration of multi-valued variants where it is valid to choose none of the values. This automatically defines "none" as an option (exclusive with all other choices); this value does not appear when iterating over the variant's values, for example in "with_or_without" (which constructs autotools option strings from variant values). * The "disjoint_sets" and "any_combination_of" methods return an object which tracks the possible values. It is also possible to indicate that some of these values do not correspond to options understood by the package's build system, such that methods like "with_or_without" will not define options for those values (this occurs automatically for "none") * Add documentation for usage of new functions for specifying multi-valued variants --- lib/spack/docs/packaging_guide.rst | 168 ++++++++++++++++ lib/spack/spack/build_systems/autotools.py | 10 +- lib/spack/spack/build_systems/cuda.py | 12 +- lib/spack/spack/directives.py | 65 +++++-- lib/spack/spack/pkgkit.py | 3 + lib/spack/spack/test/build_systems.py | 21 +- lib/spack/spack/test/spec_semantics.py | 52 ++++- lib/spack/spack/test/variant.py | 59 ++++++ lib/spack/spack/variant.py | 179 +++++++++++++++++- .../repos/builtin.mock/packages/a/package.py | 7 +- .../packages/multivalue_variant/package.py | 6 +- .../repos/builtin/packages/adios/package.py | 5 +- .../repos/builtin/packages/axl/package.py | 2 +- .../repos/builtin/packages/glib/package.py | 5 +- .../repos/builtin/packages/kokkos/package.py | 8 +- .../repos/builtin/packages/matlab/package.py | 2 +- .../builtin/packages/mvapich2/package.py | 20 +- .../repos/builtin/packages/omega-h/package.py | 5 +- .../builtin/packages/openblas/package.py | 6 +- .../repos/builtin/packages/openmpi/package.py | 17 +- .../builtin/packages/py-patsy/package.py | 4 +- .../repos/builtin/packages/regcm/package.py | 6 +- .../repos/builtin/packages/scr/package.py | 4 +- .../repos/builtin/packages/yambo/package.py | 12 +- 24 files changed, 587 insertions(+), 91 deletions(-) diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index 1c78a396e34..641bb74fea5 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/lib/spack/docs/packaging_guide.rst @@ -1077,6 +1077,174 @@ Go cannot be used to fetch a particular commit or branch, it always downloads the head of the repository. This download method is untrusted, and is not recommended. Use another fetch strategy whenever possible. +-------- +Variants +-------- + +Many software packages can be configured to enable optional +features, which often come at the expense of additional dependencies or +longer build-times. To be flexible enough and support a wide variety of +use cases, Spack permits to expose to the end-user the ability to choose +which features should be activated in a package at the time it is installed. +The mechanism to be employed is the :py:func:`spack.directives.variant` directive. + +^^^^^^^^^^^^^^^^ +Boolean variants +^^^^^^^^^^^^^^^^ + +In their simplest form variants are boolean options specified at the package +level: + + .. code-block:: python + + class Hdf5(AutotoolsPackage): + ... + variant( + 'shared', default=True, description='Builds a shared version of the library' + ) + +with a default value and a description of their meaning / use in the package. +*Variants can be tested in any context where a spec constraint is expected.* +In the example above the ``shared`` variant is tied to the build of shared dynamic +libraries. To pass the right option at configure time we can branch depending on +its value: + + .. code-block:: python + + def configure_args(self): + ... + if '+shared' in self.spec: + extra_args.append('--enable-shared') + else: + extra_args.append('--disable-shared') + extra_args.append('--enable-static-exec') + +As explained in :ref:`basic-variants` the constraint ``+shared`` means +that the boolean variant is set to ``True``, while ``~shared`` means it is set +to ``False``. +Another common example is the optional activation of an extra dependency +which requires to use the variant in the ``when`` argument of +:py:func:`spack.directives.depends_on`: + + .. code-block:: python + + class Hdf5(AutotoolsPackage): + ... + variant('szip', default=False, description='Enable szip support') + depends_on('szip', when='+szip') + +as shown in the snippet above where ``szip`` is modeled to be an optional +dependency of ``hdf5``. + +^^^^^^^^^^^^^^^^^^^^^ +Multi-valued variants +^^^^^^^^^^^^^^^^^^^^^ + +If need be, Spack can go beyond Boolean variants and permit an arbitrary +number of allowed values. This might be useful when modeling +options that are tightly related to each other. +The values in this case are passed to the :py:func:`spack.directives.variant` +directive as a tuple: + + .. code-block:: python + + class Blis(Package): + ... + variant( + 'threads', default='none', description='Multithreading support', + values=('pthreads', 'openmp', 'none'), multi=False + ) + +In the example above the argument ``multi`` is set to ``False`` to indicate +that only one among all the variant values can be active at any time. This +constraint is enforced by the parser and an error is emitted if a user +specifies two or more values at the same time: + + .. code-block:: console + + $ spack spec blis threads=openmp,pthreads + Input spec + -------------------------------- + blis threads=openmp,pthreads + + Concretized + -------------------------------- + ==> Error: multiple values are not allowed for variant "threads" + +Another useful note is that *Python's* ``None`` *is not allowed as a default value* +and therefore it should not be used to denote that no feature was selected. +Users should instead select another value, like ``'none'``, and handle it explicitly +within the package recipe if need be: + + .. code-block:: python + + if self.spec.variants['threads'].value == 'none': + options.append('--no-threads') + +In cases where multiple values can be selected at the same time ``multi`` should +be set to ``True``: + + .. code-block:: python + + class Gcc(AutotoolsPackage): + ... + variant( + 'languages', default='c,c++,fortran', + values=('ada', 'brig', 'c', 'c++', 'fortran', + 'go', 'java', 'jit', 'lto', 'objc', 'obj-c++'), + multi=True, + description='Compilers and runtime libraries to build' + ) + +Within a package recipe a multi-valued variant is tested using a ``key=value`` syntax: + + .. code-block:: python + + if 'languages=jit' in spec: + options.append('--enable-host-shared') + +""""""""""""""""""""""""""""""""""""""""""" +Complex validation logic for variant values +""""""""""""""""""""""""""""""""""""""""""" +To cover complex use cases, the :py:func:`spack.directives.variant` directive +could accept as the ``values`` argument a full-fledged object which has +``default`` and other arguments of the directive embedded as attributes. + +An example, already implemented in Spack's core, is :py:class:`spack.variant.DisjointSetsOfValues`. +This class is used to implement a few convenience functions, like +:py:func:`spack.variant.any_combination_of`: + + .. code-block:: python + + class Adios(AutotoolsPackage): + ... + variant( + 'staging', + values=any_combination_of('flexpath', 'dataspaces'), + description='Enable dataspaces and/or flexpath staging transports' + ) + +that allows any combination of the specified values, and also allows the +user to specify ``'none'`` (as a string) to choose none of them. +The objects returned by these functions can be modified at will by chaining +method calls to change the default value, customize the error message or +other similar operations: + + .. code-block:: python + + class Mvapich2(AutotoolsPackage): + ... + variant( + 'process_managers', + description='List of the process managers to activate', + values=disjoint_sets( + ('auto',), ('slurm',), ('hydra', 'gforker', 'remshell') + ).prohibit_empty_set().with_error( + "'slurm' or 'auto' cannot be activated along with " + "other process managers" + ).with_default('auto').with_non_feature_values('auto'), + ) + ------------------------------------ Resources (expanding extra tarballs) ------------------------------------ diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py index 12fe7ae2109..3673fb7cce4 100644 --- a/lib/spack/spack/build_systems/autotools.py +++ b/lib/spack/spack/build_systems/autotools.py @@ -359,9 +359,17 @@ def _activate_or_not( options = [(name, condition in spec)] else: condition = '{name}={value}' + # "feature_values" is used to track values which correspond to + # features which can be enabled or disabled as understood by the + # 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[name].values, 'feature_values', None + ) or self.variants[name].values + options = [ (value, condition.format(name=name, value=value) in spec) - for value in self.variants[name].values + for value in feature_values ] # For each allowed value in the list of values diff --git a/lib/spack/spack/build_systems/cuda.py b/lib/spack/spack/build_systems/cuda.py index 1cecdf2ccda..97dba30a16a 100644 --- a/lib/spack/spack/build_systems/cuda.py +++ b/lib/spack/spack/build_systems/cuda.py @@ -5,8 +5,11 @@ from spack.package import PackageBase from spack.directives import depends_on, variant, conflicts + import platform +import spack.variant + class CudaPackage(PackageBase): """Auxiliary class which contains CUDA variant, dependencies and conflicts @@ -19,11 +22,12 @@ class CudaPackage(PackageBase): description='Build with CUDA') # see http://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#gpu-feature-list # https://developer.nvidia.com/cuda-gpus - variant('cuda_arch', default=None, + variant('cuda_arch', description='CUDA architecture', - values=('20', '30', '32', '35', '50', '52', '53', '60', '61', - '62', '70'), - multi=True) + values=spack.variant.any_combination_of( + '20', '30', '32', '35', '50', '52', '53', '60', '61', + '62', '70' + )) # see http://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#nvcc-examples # and http://llvm.org/docs/CompileCudaWithLLVM.html#compiling-cuda-code diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index d167f856e20..08df20ae88f 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -34,6 +34,7 @@ class OpenMpi(Package): from six import string_types import llnl.util.lang +import llnl.util.tty.color import spack.error import spack.patch @@ -439,7 +440,7 @@ def variant( default=None, description='', values=None, - multi=False, + multi=None, validator=None): """Define a variant for the package. Packager can specify a default value as well as a text description. @@ -456,23 +457,65 @@ def variant( multi (bool): if False only one value per spec is allowed for this variant validator (callable): optional group validator to enforce additional - logic. It receives a tuple of values and should raise an instance - of SpackError if the group doesn't meet the additional constraints - """ - if name in reserved_names: - raise ValueError("The variant name '%s' is reserved by Spack" % name) + 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 + Raises: + DirectiveError: if arguments passed to the directive are invalid + """ + def format_error(msg, pkg): + msg += " @*r{{[{0}, variant '{1}']}}" + return llnl.util.tty.color.colorize(msg.format(pkg.name, name)) + + if name in reserved_names: + def _raise_reserved_name(pkg): + msg = "The name '%s' is reserved by Spack" % name + raise DirectiveError(format_error(msg, pkg)) + return _raise_reserved_name + + # Ensure we have a sequence of allowed variant values, or a + # predicate for it. if values is None: - if default in (True, False) or (type(default) is str and - default.upper() in ('TRUE', 'FALSE')): + if str(default).upper() in ('TRUE', 'FALSE'): values = (True, False) else: values = lambda x: True - if default is None: - default = False if values == (True, False) else '' + # The object defining variant values might supply its own defaults for + # all the other arguments. Ensure we have no conflicting definitions + # in place. + for argument in ('default', 'multi', 'validator'): + # TODO: we can consider treating 'default' differently from other + # TODO: attributes and let a packager decide whether to use the fluent + # TODO: interface or the directive argument + if hasattr(values, argument) and locals()[argument] is not None: + def _raise_argument_error(pkg): + msg = "Remove specification of {0} argument: it is handled " \ + "by an attribute of the 'values' argument" + raise DirectiveError(format_error(msg.format(argument), pkg)) + return _raise_argument_error + + # Allow for the object defining the allowed values to supply its own + # default value and group validator, say if it supports multiple values. + default = getattr(values, 'default', default) + validator = getattr(values, 'validator', validator) + multi = getattr(values, 'multi', bool(multi)) + + # Here we sanitize against a default value being either None + # or the empty string, as the former indicates that a default + # was not set while the latter will make the variant unparsable + # from the command line + if default is None or default == '': + def _raise_default_not_set(pkg): + if default is None: + msg = "either a default was not explicitly set, " \ + "or 'None' was used" + elif default == '': + msg = "the default cannot be an empty string" + raise DirectiveError(format_error(msg, pkg)) + return _raise_default_not_set - default = default description = str(description).strip() def _execute_variant(pkg): diff --git a/lib/spack/spack/pkgkit.py b/lib/spack/spack/pkgkit.py index d1a39c1a315..85c1ee264e1 100644 --- a/lib/spack/spack/pkgkit.py +++ b/lib/spack/spack/pkgkit.py @@ -47,3 +47,6 @@ from spack.package import \ install_dependency_symlinks, flatten_dependencies, \ DependencyConflictError, InstallError, ExternalPackageError + +from spack.variant import any_combination_of, auto_or_any_combination_of +from spack.variant import disjoint_sets diff --git a/lib/spack/spack/test/build_systems.py b/lib/spack/spack/test/build_systems.py index a2e684aecfa..1932d1bbc21 100644 --- a/lib/spack/spack/test/build_systems.py +++ b/lib/spack/spack/test/build_systems.py @@ -123,8 +123,11 @@ def test_with_or_without(self): s.concretize() pkg = spack.repo.get(s) - # Called without parameters options = pkg.with_or_without('foo') + + # Ensure that values that are not representing a feature + # are not used by with_or_without + assert '--without-none' not in options assert '--with-bar' in options assert '--without-baz' in options assert '--no-fee' in options @@ -133,14 +136,30 @@ def activate(value): return 'something' options = pkg.with_or_without('foo', activation_value=activate) + assert '--without-none' not in options assert '--with-bar=something' in options assert '--without-baz' in options assert '--no-fee' in options options = pkg.enable_or_disable('foo') + assert '--disable-none' not in options assert '--enable-bar' in options assert '--disable-baz' in options assert '--disable-fee' in options options = pkg.with_or_without('bvv') assert '--with-bvv' in options + + def test_none_is_allowed(self): + s = Spec('a foo=none') + s.concretize() + pkg = spack.repo.get(s) + + options = pkg.with_or_without('foo') + + # Ensure that values that are not representing a feature + # are not used by with_or_without + assert '--with-none' not in options + assert '--without-bar' in options + assert '--without-baz' in options + assert '--no-fee' in options diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 9ff5795e306..4ad9c5f9c7d 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -3,7 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) -import spack.architecture +import sys import pytest from spack.spec import Spec, UnsatisfiableSpecError @@ -11,6 +11,10 @@ from spack.variant import InvalidVariantValueError from spack.variant import MultipleValuesInExclusiveVariantError +import spack.architecture +import spack.directives +import spack.error + def target_factory(spec_string, target_concrete): spec = Spec(spec_string) if spec_string else Spec() @@ -785,3 +789,49 @@ def test_spec_flags_maintain_order(self): s.compiler_flags[x] == ['-O0', '-g'] for x in ('cflags', 'cxxflags', 'fflags') ) + + def test_any_combination_of(self): + # Test that using 'none' and another value raise during concretization + spec = Spec('multivalue_variant foo=none,bar') + with pytest.raises(spack.error.SpecError) as exc_info: + spec.concretize() + + assert "is mutually exclusive with any of the" in str(exc_info.value) + + @pytest.mark.skipif( + sys.version_info[0] == 2, reason='__wrapped__ requires python 3' + ) + def test_errors_in_variant_directive(self): + variant = spack.directives.variant.__wrapped__ + + class Pkg(object): + name = 'PKG' + + # We can't use names that are reserved by Spack + fn = variant('patches') + with pytest.raises(spack.directives.DirectiveError) as exc_info: + fn(Pkg()) + assert "The name 'patches' is reserved" in str(exc_info.value) + + # We can't have conflicting definitions for arguments + fn = variant( + 'foo', values=spack.variant.any_combination_of('fee', 'foom'), + default='bar' + ) + with pytest.raises(spack.directives.DirectiveError) as exc_info: + fn(Pkg()) + assert " it is handled by an attribute of the 'values' " \ + "argument" in str(exc_info.value) + + # We can't leave None as a default value + fn = variant('foo', default=None) + with pytest.raises(spack.directives.DirectiveError) as exc_info: + fn(Pkg()) + assert "either a default was not explicitly set, or 'None' was used"\ + in str(exc_info.value) + + # We can't use an empty string as a default value + fn = variant('foo', default='') + with pytest.raises(spack.directives.DirectiveError) as exc_info: + fn(Pkg()) + assert "the default cannot be an empty string" in str(exc_info.value) diff --git a/lib/spack/spack/test/variant.py b/lib/spack/spack/test/variant.py index c28bdf7da89..2a937422d3a 100644 --- a/lib/spack/spack/test/variant.py +++ b/lib/spack/spack/test/variant.py @@ -13,6 +13,9 @@ from spack.variant import InconsistentValidationError from spack.variant import MultipleValuesInExclusiveVariantError from spack.variant import InvalidVariantValueError, DuplicateVariantError +from spack.variant import disjoint_sets + +import spack.error class TestMultiValuedVariant(object): @@ -692,3 +695,59 @@ def test_str(self): c['feebar'] = SingleValuedVariant('feebar', 'foo') c['shared'] = BoolValuedVariant('shared', True) assert str(c) == ' feebar=foo foo=bar,baz foobar=fee +shared' + + +def test_disjoint_set_initialization_errors(): + # Constructing from non-disjoint sets should raise an exception + with pytest.raises(spack.error.SpecError) as exc_info: + disjoint_sets(('a', 'b'), ('b', 'c')) + assert 'sets in input must be disjoint' in str(exc_info.value) + + # A set containing the reserved item 'none' along with other items + # should raise an exception + with pytest.raises(spack.error.SpecError) as exc_info: + disjoint_sets(('a', 'b'), ('none', 'c')) + assert "The value 'none' represents the empty set," in str(exc_info.value) + + +def test_disjoint_set_initialization(): + # Test that no error is thrown when the sets are disjoint + d = disjoint_sets(('a',), ('b', 'c'), ('e', 'f')) + + assert d.default is 'none' + assert d.multi is True + assert set(x for x in d) == set(['none', 'a', 'b', 'c', 'e', 'f']) + + +def test_disjoint_set_fluent_methods(): + # Construct an object without the empty set + d = disjoint_sets(('a',), ('b', 'c'), ('e', 'f')).prohibit_empty_set() + assert set(('none',)) not in d.sets + + # Call this 2 times to check that no matter whether + # the empty set was allowed or not before, the state + # returned is consistent. + for _ in range(2): + d = d.allow_empty_set() + assert set(('none',)) in d.sets + assert 'none' in d + assert 'none' in [x for x in d] + assert 'none' in d.feature_values + + # Marking a value as 'non-feature' removes it from the + # list of feature values, but not for the items returned + # when iterating over the object. + d = d.with_non_feature_values('none') + assert 'none' in d + assert 'none' in [x for x in d] + assert 'none' not in d.feature_values + + # Call this 2 times to check that no matter whether + # the empty set was allowed or not before, the state + # returned is consistent. + for _ in range(2): + d = d.prohibit_empty_set() + assert set(('none',)) not in d.sets + assert 'none' not in d + assert 'none' not in [x for x in d] + assert 'none' not in d.feature_values diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py index 05e9ac810d6..2874eeb60aa 100644 --- a/lib/spack/spack/variant.py +++ b/lib/spack/spack/variant.py @@ -9,14 +9,21 @@ import functools import inspect +import itertools import re from six import StringIO +import llnl.util.tty.color import llnl.util.lang as lang import spack.directives import spack.error as error +try: + from collections.abc import Sequence +except ImportError: + from collections import Sequence + class Variant(object): """Represents a variant in a package, as declared in the @@ -71,8 +78,8 @@ def isa_type(v): else: # Otherwise assume values is the set of allowed explicit values - self.values = tuple(values) - allowed = self.values + (self.default,) + self.values = values + allowed = tuple(self.values) + (self.default,) self.single_value_validator = lambda x: x in allowed self.multi = multi @@ -118,7 +125,7 @@ def validate_or_raise(self, vspec, pkg=None): # Validate the group of values if needed if self.group_validator is not None: - self.group_validator(value) + self.group_validator(pkg.name, self.name, value) @property def allowed_values(self): @@ -598,6 +605,172 @@ def substitute_abstract_variants(spec): spec.variants.substitute(new_variant) +# The class below inherit from Sequence to disguise as a tuple and comply +# with the semantic expected by the 'values' argument of the variant directive +class DisjointSetsOfValues(Sequence): + """Allows combinations from one of many mutually exclusive sets. + + The value ``('none',)`` is reserved to denote the empty set + and therefore no other set can contain the item ``'none'``. + + Args: + *sets (list of tuples): mutually exclusive sets of values + """ + + _empty_set = set(('none',)) + + def __init__(self, *sets): + self.sets = [set(x) for x in sets] + + # 'none' is a special value and can appear only in a set of + # a single element + if any('none' in s and s != set(('none',)) for s in self.sets): + raise error.SpecError("The value 'none' represents the empty set," + " and must appear alone in a set. Use the " + "method 'allow_empty_set' to add it.") + + # Sets should not intersect with each other + if any(s1 & s2 for s1, s2 in itertools.combinations(self.sets, 2)): + raise error.SpecError('sets in input must be disjoint') + + #: Attribute used to track values which correspond to + #: features which can be enabled or disabled as understood by the + #: package's build system. + self.feature_values = tuple(itertools.chain.from_iterable(self.sets)) + self.default = None + self.multi = True + self.error_fmt = "this variant accepts combinations of values from " \ + "exactly one of the following sets '{values}' " \ + "@*r{{[{package}, variant '{variant}']}}" + + def with_default(self, default): + """Sets the default value and returns self.""" + self.default = default + return self + + def with_error(self, error_fmt): + """Sets the error message format and returns self.""" + self.error_fmt = error_fmt + return self + + def with_non_feature_values(self, *values): + """Marks a few values as not being tied to a feature.""" + self.feature_values = tuple( + x for x in self.feature_values if x not in values + ) + return self + + def allow_empty_set(self): + """Adds the empty set to the current list of disjoint sets.""" + if self._empty_set in self.sets: + return self + + # Create a new object to be returned + object_with_empty_set = type(self)(('none',), *self.sets) + object_with_empty_set.error_fmt = self.error_fmt + object_with_empty_set.feature_values = self.feature_values + ('none', ) + return object_with_empty_set + + def prohibit_empty_set(self): + """Removes the empty set from the current list of disjoint sets.""" + if self._empty_set not in self.sets: + return self + + # Create a new object to be returned + sets = [s for s in self.sets if s != self._empty_set] + object_without_empty_set = type(self)(*sets) + object_without_empty_set.error_fmt = self.error_fmt + object_without_empty_set.feature_values = tuple( + x for x in self.feature_values if x != 'none' + ) + return object_without_empty_set + + def __getitem__(self, idx): + return tuple(itertools.chain.from_iterable(self.sets))[idx] + + def __len__(self): + return len(itertools.chain.from_iterable(self.sets)) + + @property + def validator(self): + def _disjoint_set_validator(pkg_name, variant_name, values): + # If for any of the sets, all the values are in it return True + if any(all(x in s for x in values) for s in self.sets): + return + + format_args = { + 'variant': variant_name, 'package': pkg_name, 'values': values + } + msg = self.error_fmt + \ + " @*r{{[{package}, variant '{variant}']}}" + msg = llnl.util.tty.color.colorize(msg.format(**format_args)) + raise error.SpecError(msg) + return _disjoint_set_validator + + +def _a_single_value_or_a_combination(single_value, *values): + error = "the value '" + single_value + \ + "' is mutually exclusive with any of the other values" + return DisjointSetsOfValues( + (single_value,), values + ).with_default(single_value).with_error(error).\ + with_non_feature_values(single_value) + + +# TODO: The factories below are used by package writers to set values of +# TODO: multi-valued variants. It could be worthwhile to gather them in +# TODO: a common namespace (like 'multi') in the future. + + +def any_combination_of(*values): + """Multi-valued variant that allows any combination of the specified + values, and also allows the user to specify 'none' (as a string) to choose + none of them. + + It is up to the package implementation to handle the value 'none' + specially, if at all. + + Args: + *values: allowed variant values + + Returns: + a properly initialized instance of DisjointSetsOfValues + """ + return _a_single_value_or_a_combination('none', *values) + + +def auto_or_any_combination_of(*values): + """Multi-valued variant that allows any combination of a set of values + (but not the empty set) or 'auto'. + + Args: + *values: allowed variant values + + Returns: + a properly initialized instance of DisjointSetsOfValues + """ + return _a_single_value_or_a_combination('auto', *values) + + +#: Multi-valued variant that allows any combination picking +#: from one of multiple disjoint sets +def disjoint_sets(*sets): + """Multi-valued variant that allows any combination picking from one + of multiple disjoint sets of values, and also allows the user to specify + 'none' (as a string) to choose none of them. + + It is up to the package implementation to handle the value 'none' + specially, if at all. + + Args: + *sets: + + Returns: + a properly initialized instance of DisjointSetsOfValues + """ + return DisjointSetsOfValues(*sets).allow_empty_set().with_default('none') + + class DuplicateVariantError(error.SpecError): """Raised when the same variant occurs in a spec twice.""" diff --git a/var/spack/repos/builtin.mock/packages/a/package.py b/var/spack/repos/builtin.mock/packages/a/package.py index 2474c0f61b7..7e5a6f72e66 100644 --- a/var/spack/repos/builtin.mock/packages/a/package.py +++ b/var/spack/repos/builtin.mock/packages/a/package.py @@ -16,11 +16,8 @@ class A(AutotoolsPackage): version('2.0', '2.0_a_hash') variant( - 'foo', - values=('bar', 'baz', 'fee'), - default='bar', - description='', - multi=True + 'foo', description='', + values=any_combination_of('bar', 'baz', 'fee').with_default('bar'), ) variant( diff --git a/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py b/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py index 442a7562336..3ae05c5110c 100644 --- a/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py +++ b/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py @@ -17,10 +17,8 @@ class MultivalueVariant(Package): variant('debug', default=False, description='Debug variant') variant( - 'foo', - description='Multi-valued variant', - values=('bar', 'baz', 'barbaz'), - multi=True + 'foo', description='Multi-valued variant', + values=any_combination_of('bar', 'baz', 'barbaz'), ) variant( diff --git a/var/spack/repos/builtin/packages/adios/package.py b/var/spack/repos/builtin/packages/adios/package.py index 0aeb364c0fd..d4ea3258e22 100644 --- a/var/spack/repos/builtin/packages/adios/package.py +++ b/var/spack/repos/builtin/packages/adios/package.py @@ -61,10 +61,7 @@ class Adios(AutotoolsPackage): variant('netcdf', default=False, description='Enable netcdf support') variant( - 'staging', - default=None, - values=('flexpath', 'dataspaces'), - multi=True, + 'staging', values=any_combination_of('flexpath', 'dataspaces'), description='Enable dataspaces and/or flexpath staging transports' ) diff --git a/var/spack/repos/builtin/packages/axl/package.py b/var/spack/repos/builtin/packages/axl/package.py index 869232cfe7f..a524ca06430 100644 --- a/var/spack/repos/builtin/packages/axl/package.py +++ b/var/spack/repos/builtin/packages/axl/package.py @@ -7,7 +7,7 @@ from spack.error import SpackError -def async_api_validator(values): +def async_api_validator(pkg_name, variant_name, values): if 'none' in values and len(values) != 1: raise SpackError("The value 'none' is not usable" " with other async_api values.") diff --git a/var/spack/repos/builtin/packages/glib/package.py b/var/spack/repos/builtin/packages/glib/package.py index 414b07c6ea5..b36ec9362bd 100644 --- a/var/spack/repos/builtin/packages/glib/package.py +++ b/var/spack/repos/builtin/packages/glib/package.py @@ -34,10 +34,7 @@ class Glib(AutotoolsPackage): variant('libmount', default=False, description='Build with libmount support') variant( - 'tracing', - default='', - values=('dtrace', 'systemtap'), - multi=True, + 'tracing', values=any_combination_of('dtrace', 'systemtap'), description='Enable tracing support' ) diff --git a/var/spack/repos/builtin/packages/kokkos/package.py b/var/spack/repos/builtin/packages/kokkos/package.py index d889280b28b..d73d0ae74aa 100644 --- a/var/spack/repos/builtin/packages/kokkos/package.py +++ b/var/spack/repos/builtin/packages/kokkos/package.py @@ -71,7 +71,7 @@ class Kokkos(Package): # Host architecture variant variant( 'host_arch', - default=None, + default='none', values=('AMDAVX', 'ARMv80', 'ARMv81', 'ARMv8-ThunderX', 'Power7', 'Power8', 'Power9', 'WSM', 'SNB', 'HSW', 'BDW', 'SKX', 'KNC', 'KNL'), @@ -81,7 +81,7 @@ class Kokkos(Package): # GPU architecture variant variant( 'gpu_arch', - default=None, + default='none', values=gpu_values, description='Set the GPU architecture to use' ) @@ -159,9 +159,9 @@ def install(self, spec, prefix): host_arch = spec.variants['host_arch'].value # GPU architectures gpu_arch = spec.variants['gpu_arch'].value - if host_arch: + if host_arch != 'none': arch_args.append(host_arch) - if gpu_arch: + if gpu_arch != 'none': arch_args.append(gpu_arch) # Combined architecture flags if arch_args: diff --git a/var/spack/repos/builtin/packages/matlab/package.py b/var/spack/repos/builtin/packages/matlab/package.py index d0b242d5189..639ec65890c 100644 --- a/var/spack/repos/builtin/packages/matlab/package.py +++ b/var/spack/repos/builtin/packages/matlab/package.py @@ -35,7 +35,7 @@ class Matlab(Package): variant( 'key', - default='', + default='', values=lambda x: True, # Anything goes as a key description='The file installation key to use' ) diff --git a/var/spack/repos/builtin/packages/mvapich2/package.py b/var/spack/repos/builtin/packages/mvapich2/package.py index 8493e583650..3cf2dba4088 100644 --- a/var/spack/repos/builtin/packages/mvapich2/package.py +++ b/var/spack/repos/builtin/packages/mvapich2/package.py @@ -6,14 +6,6 @@ import sys from spack import * -from spack.error import SpackError - - -def _process_manager_validator(values): - if len(values) > 1 and 'slurm' in values: - raise SpackError( - 'slurm cannot be activated along with other process managers' - ) class Mvapich2(AutotoolsPackage): @@ -70,9 +62,12 @@ class Mvapich2(AutotoolsPackage): variant( 'process_managers', description='List of the process managers to activate', - values=('slurm', 'hydra', 'gforker', 'remshell'), - multi=True, - validator=_process_manager_validator + values=disjoint_sets( + ('auto',), ('slurm',), ('hydra', 'gforker', 'remshell') + ).prohibit_empty_set().with_error( + "'slurm' or 'auto' cannot be activated along with " + "other process managers" + ).with_default('auto').with_non_feature_values('auto'), ) variant( @@ -94,8 +89,7 @@ class Mvapich2(AutotoolsPackage): variant( 'file_systems', description='List of the ROMIO file systems to activate', - values=('lustre', 'gpfs', 'nfs', 'ufs'), - multi=True + values=auto_or_any_combination_of('lustre', 'gpfs', 'nfs', 'ufs'), ) depends_on('findutils', type='build') diff --git a/var/spack/repos/builtin/packages/omega-h/package.py b/var/spack/repos/builtin/packages/omega-h/package.py index c28c10eea64..d2bee167330 100644 --- a/var/spack/repos/builtin/packages/omega-h/package.py +++ b/var/spack/repos/builtin/packages/omega-h/package.py @@ -3,8 +3,6 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) -from spack import * - class OmegaH(CMakePackage): """Omega_h is a C++11 library providing data structures and algorithms @@ -28,8 +26,7 @@ class OmegaH(CMakePackage): variant('shared', default=True, description='Build shared libraries') variant('mpi', default=True, description='Activates MPI support') variant('zlib', default=True, description='Activates ZLib support') - variant('trilinos', default=False, description='Use Teuchos and Kokkos') - variant('build_type', default='') + variant('trilinos', default=True, description='Use Teuchos and Kokkos') variant('throw', default=False, description='Errors throw exceptions instead of abort') variant('examples', default=False, description='Compile examples') variant('optimize', default=True, description='Compile C++ with optimization') diff --git a/var/spack/repos/builtin/packages/openblas/package.py b/var/spack/repos/builtin/packages/openblas/package.py index 70ff12babfd..ed8bba51ff0 100644 --- a/var/spack/repos/builtin/packages/openblas/package.py +++ b/var/spack/repos/builtin/packages/openblas/package.py @@ -38,8 +38,8 @@ class Openblas(MakefilePackage): variant('ilp64', default=False, description='64 bit integers') variant('pic', default=True, description='Build position independent code') - variant('cpu_target', default='', - description='Set CPU target architecture (leave empty for ' + variant('cpu_target', default='auto', + description='Set CPU target architecture (leave empty for ' 'autodetection; GENERIC, SSE_GENERIC, NEHALEM, ...)') variant( @@ -150,7 +150,7 @@ def make_defs(self): 'NUM_THREADS=64', # OpenBLAS stores present no of CPUs as max ] - if self.spec.variants['cpu_target'].value: + if self.spec.variants['cpu_target'].value != 'auto': make_defs += [ 'TARGET={0}'.format(self.spec.variants['cpu_target'].value) ] diff --git a/var/spack/repos/builtin/packages/openmpi/package.py b/var/spack/repos/builtin/packages/openmpi/package.py index 2925048d39d..5eda1d06581 100644 --- a/var/spack/repos/builtin/packages/openmpi/package.py +++ b/var/spack/repos/builtin/packages/openmpi/package.py @@ -7,8 +7,6 @@ import os import sys -from spack import * - def _verbs_dir(): """Try to find the directory where the OpenFabrics verbs package is @@ -180,20 +178,17 @@ class Openmpi(AutotoolsPackage): patch('btl_vader.patch', when='@3.1.0:3.1.2') fabrics = ('psm', 'psm2', 'verbs', 'mxm', 'ucx', 'libfabric') - variant( - 'fabrics', - default=None if _verbs_dir() is None else 'verbs', + 'fabrics', values=auto_or_any_combination_of(*fabrics).with_default( + 'auto' if _verbs_dir() is None else 'verbs' + ), description="List of fabrics that are enabled", - values=fabrics, - multi=True ) + schedulers = ('alps', 'lsf', 'tm', 'slurm', 'sge', 'loadleveler') variant( - 'schedulers', - description='List of schedulers for which support is enabled', - values=('alps', 'lsf', 'tm', 'slurm', 'sge', 'loadleveler'), - multi=True + 'schedulers', values=auto_or_any_combination_of(*schedulers), + description='List of schedulers for which support is enabled' ) # Additional support options diff --git a/var/spack/repos/builtin/packages/py-patsy/package.py b/var/spack/repos/builtin/packages/py-patsy/package.py index 326825b1f5e..6ffd4e19b91 100644 --- a/var/spack/repos/builtin/packages/py-patsy/package.py +++ b/var/spack/repos/builtin/packages/py-patsy/package.py @@ -3,8 +3,6 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) -from spack import * - class PyPatsy(PythonPackage): """A Python package for describing statistical models and for @@ -15,7 +13,7 @@ class PyPatsy(PythonPackage): version('0.4.1', '9445f29e3426d1ed30d683a1e1453f84') - variant('splines', description="Offers spline related functions") + variant('splines', default=False, description="Offers spline related functions") depends_on('py-setuptools', type='build') depends_on('py-numpy', type=('build', 'run')) diff --git a/var/spack/repos/builtin/packages/regcm/package.py b/var/spack/repos/builtin/packages/regcm/package.py index a732f6c3430..f9a1b7c65c4 100644 --- a/var/spack/repos/builtin/packages/regcm/package.py +++ b/var/spack/repos/builtin/packages/regcm/package.py @@ -25,8 +25,10 @@ class Regcm(AutotoolsPackage): # producing a so-called fat binary. Unfortunately, gcc builds only the last # architecture provided (in the configure), so we allow a single arch. extensions = ('knl', 'skl', 'bdw', 'nhl') - variant('extension', default=None, values=extensions, multi=True, - description='Build extensions for a specific Intel architecture.') + variant( + 'extension', values=any_combination_of(extensions), + description='Build extensions for a specific Intel architecture.' + ) depends_on('netcdf') depends_on('netcdf-fortran') diff --git a/var/spack/repos/builtin/packages/scr/package.py b/var/spack/repos/builtin/packages/scr/package.py index 1f33bf7ca20..3cf94d7471e 100644 --- a/var/spack/repos/builtin/packages/scr/package.py +++ b/var/spack/repos/builtin/packages/scr/package.py @@ -47,7 +47,7 @@ class Scr(CMakePackage): variant('scr_config', default='scr.conf', description='Location for SCR to find its system config file. ' 'May be either absolute or relative to the install prefix') - variant('copy_config', default=None, + variant('copy_config', default='none', description='Location from which to copy SCR system config file. ' 'Must be an absolute path.') @@ -130,7 +130,7 @@ def cmake_args(self): @run_after('install') def copy_config(self): spec = self.spec - if spec.variants['copy_config'].value: + if spec.variants['copy_config'].value != 'none': dest_path = self.get_abs_path_rel_prefix( spec.variants['scr_config'].value) install(spec.variants['copy_config'].value, dest_path) diff --git a/var/spack/repos/builtin/packages/yambo/package.py b/var/spack/repos/builtin/packages/yambo/package.py index 04b8272aa62..5e70c358eac 100644 --- a/var/spack/repos/builtin/packages/yambo/package.py +++ b/var/spack/repos/builtin/packages/yambo/package.py @@ -26,19 +26,13 @@ class Yambo(AutotoolsPackage): variant('dp', default=False, description='Enable double precision') variant( - 'profile', - values=('time', 'memory'), - default='', - description='Activate profiling of specific sections', - multi=True + 'profile', values=any_combination_of('time', 'memory'), + description='Activate profiling of specific sections' ) variant( - 'io', - values=('iotk', 'etsf-io'), - default='', + 'io', values=any_combination_of('iotk', 'etsf-io'), description='Activate support for different io formats (requires network access)', # noqa - multi=True ) # MPI + OpenMP parallelism