'with_or_without' accepts bool variants

Fixes #4112

This commit extends the support of the AutotoolsPackage methods
`with_or_without` and `enable_or_disable` to bool-valued variants. It
also defines for those functions a convenience short-cut if the
activation parameter is the prefix of a spec (like in
`--with-{pkg}={prefix}`).

This commit also includes:

* Updates to viennarna and adios accordingly: they have been modified to
  use `enable_or_disable` and `with_or_without`
* Improved docstrings in `autotools.py`. Raise `KeyError` if name is
  not a variant.

5 files changed, 202 insertions, 109 deletions

diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index afabb90820..b78d3ed026 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -291,49 +291,161 @@ class AutotoolsPackage(PackageBase):
             self._if_make_target_execute('test')
             self._if_make_target_execute('check')
 
-    def _activate_or_not(self, active, inactive, name, active_parameters=None):
+    def _activate_or_not(
+            self,
+            name,
+            activation_word,
+            deactivation_word,
+            activation_value=None
+    ):
+        """This function contains the current implementation details of
+        :py:meth:`~.AutotoolsPackage.with_or_without` and
+        :py:meth:`~.AutotoolsPackage.enable_or_disable`.
+
+        Args:
+            name (str): name of the variant that is being processed
+            activation_word (str): the default activation word ('with' in the
+                case of ``with_or_without``)
+            deactivation_word (str): the default deactivation word ('without'
+                in the case of ``with_or_without``)
+            activation_value (callable): callable that accepts a single
+                value. This value is either one of the allowed values for a
+                multi-valued variant or the name of a bool-valued variant.
+                Returns the parameter to be used when the value is activated.
+
+                The special value 'prefix' can also be assigned and will return
+                ``spec[name].prefix`` as activation parameter.
+
+        Examples:
+
+            Given a package with:
+
+            .. code-block:: python
+
+                variant('foo', values=('x', 'y'), description='')
+                variant('bar', default=True, description='')
+
+            calling this function like:
+
+            .. code-block:: python
+
+                _activate_or_not(
+                    'foo', 'with', 'without', activation_value='prefix'
+                )
+                _activate_or_not('bar', 'with', 'without')
+
+            will generate the following configuration options:
+
+            .. code-block:: console
+
+                --with-x=<prefix-to-x> --without-y --with-bar
+
+            for ``<spec-name> foo=x +bar``
+
+        Returns:
+            list of strings that corresponds to the activation/deactivation
+            of the variant that has been processed
+
+        Raises:
+            KeyError: if name is not among known variants
+        """
         spec = self.spec
         args = []
+
+        if activation_value == 'prefix':
+            activation_value = lambda x: spec[x].prefix
+
+        # Defensively look that the name passed as argument is among
+        # variants
+        if name not in self.variants:
+            msg = '"{0}" is not a variant of "{1}"'
+            raise KeyError(msg.format(name, self.name))
+
+        # Create a list of pairs. Each pair includes a configuration
+        # option and whether or not that option is activated
+        if set(self.variants[name].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.
+            condition = '+{name}'.format(name=name)
+            options = [(name, condition in spec)]
+        else:
+            condition = '{name}={value}'
+            options = [
+                (value, condition.format(name=name, value=value) in spec)
+                for value in self.variants[name].values
+            ]
+
         # For each allowed value in the list of values
-        for value in self.variants[name].values:
-            # Check if the value is active in the current spec
-            condition = '{name}={value}'.format(name=name, value=value)
-            activated = condition in spec
+        for option_value, activated in options:
             # Search for an override in the package for this value
-            override_name = '{0}_or_{1}_{2}'.format(active, inactive, value)
+            override_name = '{0}_or_{1}_{2}'.format(
+                activation_word, deactivation_word, option_value
+            )
             line_generator = getattr(self, override_name, None)
             # If not available use a sensible default
             if line_generator is None:
                 def _default_generator(is_activated):
                     if is_activated:
-                        line = '--{0}-{1}'.format(active, value)
-                        if active_parameters is not None and active_parameters(value):  # NOQA=ignore=E501
-                            line += '={0}'.format(active_parameters(value))
+                        line = '--{0}-{1}'.format(
+                            activation_word, option_value
+                        )
+                        if activation_value is not None and activation_value(option_value):  # NOQA=ignore=E501
+                            line += '={0}'.format(
+                                activation_value(option_value)
+                            )
                         return line
-                    return '--{0}-{1}'.format(inactive, value)
+                    return '--{0}-{1}'.format(deactivation_word, option_value)
                 line_generator = _default_generator
             args.append(line_generator(activated))
         return args
 
-    def with_or_without(self, name, active_parameters=None):
-        """Inspects the multi-valued variant 'name' and returns the configure
-        arguments that activate / deactivate the selected feature.
+    def with_or_without(self, name, activation_value=None):
+        """Inspects a variant and returns the arguments that activate
+        or deactivate the selected feature(s) for the configure options.
+
+        This function works on all type of variants. For bool-valued variants
+        it will return by default ``--with-{name}`` or ``--without-{name}``.
+        For other kinds of variants it will cycle over the allowed values and
+        return either ``--with-{value}`` or ``--without-{value}``.
+
+        If activation_value is given, then for each possible value of the
+        variant, the option ``--with-{value}=activation_value(value)`` or
+        ``--without-{value}`` will be added depending on whether or not
+        ``variant=value`` is in the spec.
 
-        :param str name: name of a valid multi-valued variant
-        :param callable active_parameters: if present accepts a single value
-            and returns the parameter to be used leading to an entry of the
-            type '--with-{name}={parameter}
+        Args:
+            name (str): name of a valid multi-valued variant
+            activation_value (callable): callable that accepts a single
+                value and returns the parameter to be used leading to an entry
+                of the type ``--with-{name}={parameter}``.
+
+                The special value 'prefix' can also be assigned and will return
+                ``spec[name].prefix`` as activation parameter.
+
+        Returns:
+            list of arguments to configure
         """
-        return self._activate_or_not(
-            'with', 'without', name, active_parameters
-        )
+        return self._activate_or_not(name, 'with', 'without', activation_value)
+
+    def enable_or_disable(self, name, activation_value=None):
+        """Same as :py:meth:`~.AutotoolsPackage.with_or_without` but substitute
+        ``with`` with ``enable`` and ``without`` with ``disable``.
+
+        Args:
+            name (str): name of a valid multi-valued variant
+            activation_value (callable): if present accepts a single value
+                and returns the parameter to be used leading to an entry of the
+                type ``--enable-{name}={parameter}``
+
+                The special value 'prefix' can also be assigned and will return
+                ``spec[name].prefix`` as activation parameter.
 
-    def enable_or_disable(self, name, active_parameters=None):
-        """Inspects the multi-valued variant 'name' and returns the configure
-        arguments that activate / deactivate the selected feature.
+        Returns:
+            list of arguments to configure
         """
         return self._activate_or_not(
-            'enable', 'disable', name, active_parameters
+            name, 'enable', 'disable', activation_value
         )
 
     run_after('install')(PackageBase._run_default_install_time_test_callbacks)
diff --git a/lib/spack/spack/test/build_systems.py b/lib/spack/spack/test/build_systems.py
index 114497bccd..9960966e2f 100644
--- a/lib/spack/spack/test/build_systems.py
+++ b/lib/spack/spack/test/build_systems.py
@@ -53,20 +53,23 @@ class TestAutotoolsPackage(object):
         pkg = spack.repo.get(s)
 
         # Called without parameters
-        l = pkg.with_or_without('foo')
-        assert '--with-bar' in l
-        assert '--without-baz' in l
-        assert '--no-fee' in l
+        options = pkg.with_or_without('foo')
+        assert '--with-bar' in options
+        assert '--without-baz' in options
+        assert '--no-fee' in options
 
         def activate(value):
             return 'something'
 
-        l = pkg.with_or_without('foo', active_parameters=activate)
-        assert '--with-bar=something' in l
-        assert '--without-baz' in l
-        assert '--no-fee' in l
+        options = pkg.with_or_without('foo', activation_value=activate)
+        assert '--with-bar=something' in options
+        assert '--without-baz' in options
+        assert '--no-fee' in options
 
-        l = pkg.enable_or_disable('foo')
-        assert '--enable-bar' in l
-        assert '--disable-baz' in l
-        assert '--disable-fee' in l
+        options = pkg.enable_or_disable('foo')
+        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
diff --git a/var/spack/repos/builtin.mock/packages/a/package.py b/var/spack/repos/builtin.mock/packages/a/package.py
index dc078d2434..463ad10055 100644
--- a/var/spack/repos/builtin.mock/packages/a/package.py
+++ b/var/spack/repos/builtin.mock/packages/a/package.py
@@ -50,6 +50,8 @@ class A(AutotoolsPackage):
         multi=False
     )
 
+    variant('bvv', default=True, description='The good old BV variant')
+
     depends_on('b', when='foobar=bar')
 
     def with_or_without_fee(self, activated):
diff --git a/var/spack/repos/builtin/packages/adios/package.py b/var/spack/repos/builtin/packages/adios/package.py
index 88721a477a..91188b8e4e 100644
--- a/var/spack/repos/builtin/packages/adios/package.py
+++ b/var/spack/repos/builtin/packages/adios/package.py
@@ -61,9 +61,14 @@ class Adios(AutotoolsPackage):
     # transports and serial file converters
     variant('hdf5', default=False, description='Enable parallel HDF5 transport and serial bp2h5 converter')
     variant('netcdf', default=False, description='Enable netcdf support')
-    variant('flexpath', default=False, description='Enable flexpath transport')
-    variant('dataspaces', default=False, description='Enable dataspaces transport')
-    variant('staging', default=False, description='Enable dataspaces and flexpath staging transports')
+
+    variant(
+        'staging',
+        default=None,
+        values=('flexpath', 'dataspaces'),
+        multi=True,
+        description='Enable dataspaces and/or flexpath staging transports'
+    )
 
     depends_on('autoconf', type='build')
     depends_on('automake', type='build')
@@ -100,15 +105,27 @@ class Adios(AutotoolsPackage):
     patch('adios_1100.patch', when='@:1.10.0^hdf5@1.10:')
 
     def validate(self, spec):
-        """
-        Checks if incompatible variants have been activated at the same time
-        :param spec: spec of the package
-        :raises RuntimeError: in case of inconsistencies
+        """Checks if incompatible variants have been activated at the same time
+
+        Args:
+            spec: spec of the package
+
+        Raises:
+            RuntimeError: in case of inconsistencies
         """
         if '+fortran' in spec and not self.compiler.fc:
             msg = 'cannot build a fortran variant without a fortran compiler'
             raise RuntimeError(msg)
 
+    def with_or_without_hdf5(self, activated):
+
+        if activated:
+            return '--with-phdf5={0}'.format(
+                self.spec['hdf5'].prefix
+            )
+
+        return '--without-phdf5'
+
     def configure_args(self):
         spec = self.spec
         self.validate(spec)
@@ -118,66 +135,31 @@ class Adios(AutotoolsPackage):
             'CFLAGS={0}'.format(self.compiler.pic_flag)
         ]
 
-        if '+shared' in spec:
-            extra_args.append('--enable-shared')
+        extra_args += self.enable_or_disable('shared')
+        extra_args += self.enable_or_disable('fortran')
 
         if '+mpi' in spec:
             env['MPICC'] = spec['mpi'].mpicc
             env['MPICXX'] = spec['mpi'].mpicxx
-            extra_args.append('--with-mpi=%s' % spec['mpi'].prefix)
-        else:
-            extra_args.append('--without-mpi')
-        if '+infiniband' in spec:
-            extra_args.append('--with-infiniband')
-        else:
-            extra_args.append('--with-infiniband=no')
-
-        if '+fortran' in spec:
-            extra_args.append('--enable-fortran')
-        else:
-            extra_args.append('--disable-fortran')
+
+        extra_args += self.with_or_without('mpi', activation='prefix')
+        extra_args += self.with_or_without('infiniband')
 
         # Transforms
-        if '+zlib' in spec:
-            extra_args.append('--with-zlib=%s' % spec['zlib'].prefix)
-        else:
-            extra_args.append('--without-zlib')
-        if '+bzip2' in spec:
-            extra_args.append('--with-bzip2=%s' % spec['bzip2'].prefix)
-        else:
-            extra_args.append('--without-bzip2')
-        if '+szip' in spec:
-            extra_args.append('--with-szip=%s' % spec['szip'].prefix)
-        else:
-            extra_args.append('--without-szip')
-        if '+zfp' in spec:
-            extra_args.append('--with-zfp=%s' % spec['zfp'].prefix)
-        else:
-            extra_args.append('--without-zfp')
-        if '+sz' in spec:
-            extra_args.append('--with-sz=%s' % spec['sz'].prefix)
-        else:
-            extra_args.append('--without-sz')
+        variants = ['zlib', 'bzip2', 'szip', 'zfp', 'sz']
 
         # External I/O libraries
-        if '+hdf5' in spec:
-            extra_args.append('--with-phdf5=%s' % spec['hdf5'].prefix)
-        else:
-            extra_args.append('--without-phdf5')
-        if '+netcdf' in spec:
-            extra_args.append('--with-netcdf=%s' % spec['netcdf'].prefix)
-        else:
-            extra_args.append('--without-netcdf')
+        variants += ['hdf5', 'netcdf']
+
+        for x in variants:
+            extra_args += self.with_or_without(x, activation='prefix')
 
         # Staging transports
-        if '+flexpath' in spec or '+staging' in spec:
-            extra_args.append('--with-flexpath=%s' % spec['libevpath'].prefix)
-        else:
-            extra_args.append('--without-flexpath')
-        if '+dataspaces' in spec or '+staging' in spec:
-            extra_args.append('--with-dataspaces=%s'
-                              % spec['dataspaces'].prefix)
-        else:
-            extra_args.append('--without-dataspaces')
+        def with_staging(name):
+            if name == 'flexpath':
+                return spec['libevpath'].prefix
+            return spec[name].prefix
+
+        extra_args += self.with_or_without('staging', activation=with_staging)
 
         return extra_args
diff --git a/var/spack/repos/builtin/packages/viennarna/package.py b/var/spack/repos/builtin/packages/viennarna/package.py
index dbaff25eec..7f8f2cb18c 100644
--- a/var/spack/repos/builtin/packages/viennarna/package.py
+++ b/var/spack/repos/builtin/packages/viennarna/package.py
@@ -27,8 +27,9 @@ from spack import *
 
 class Viennarna(AutotoolsPackage):
     """The ViennaRNA Package consists of a C code library and several
-       stand-alone programs for the prediction and comparison of RNA secondary
-       structures."""
+    stand-alone programs for the prediction and comparison of RNA secondary
+    structures.
+    """
 
     homepage = "https://www.tbi.univie.ac.at/RNA/"
     url      = "https://www.tbi.univie.ac.at/RNA/download/sourcecode/2_3_x/ViennaRNA-2.3.5.tar.gz"
@@ -49,19 +50,12 @@ class Viennarna(AutotoolsPackage):
         return url.format(version.up_to(2).underscored, version)
 
     def configure_args(self):
-        args = []
-        if '+sse' in self.spec:
-            args.append('--enable-sse')
-        else:
-            args.append('--disable-sse')
-        if '~python' in self.spec:
-            args.append('--without-python')
-        else:
-            args.append('--with-python')
-        if '~perl' in self.spec:
-            args.append('--without-perl')
-        else:
-            args.append('--with-perl')
+
+        args = self.enable_or_disable('sse')
+        args += self.with_or_without('python')
+        args += self.with_or_without('perl')
+
         if 'python@3:' in self.spec:
             args.append('--with-python3')
+
         return args