variants: fixed packages reported by @adamjstewart in #4098 (#4105)

5 files changed, 85 insertions, 33 deletions

diff --git a/var/spack/repos/builtin/packages/charm/package.py b/var/spack/repos/builtin/packages/charm/package.py
index 85451eb5ae..3e84394da8 100644
--- a/var/spack/repos/builtin/packages/charm/package.py
+++ b/var/spack/repos/builtin/packages/charm/package.py
@@ -53,8 +53,12 @@ class Charm(Package):
 
     # Communication mechanisms (choose exactly one)
     # TODO: Support Blue Gene/Q PAMI, Cray GNI, Cray shmem, CUDA
-    variant('backend', default='mpi', description=(
-        'Set the backend to use (mpi, multicore, net, netlrts, verbs)'))
+    variant(
+        'backend',
+        default='mpi',
+        values=('mpi', 'multicore', 'net', 'netlrts', 'verbs'),
+        description='Set the backend to use'
+    )
 
     # Other options
     # Something is off with PAPI -- there are build errors. Maybe
diff --git a/var/spack/repos/builtin/packages/matlab/package.py b/var/spack/repos/builtin/packages/matlab/package.py
index 787b89031e..589116b08b 100644
--- a/var/spack/repos/builtin/packages/matlab/package.py
+++ b/var/spack/repos/builtin/packages/matlab/package.py
@@ -45,8 +45,19 @@ class Matlab(Package):
 
     version('R2016b', 'b0e0b688894282139fa787b5a86a5cf7')
 
-    variant('mode', default='interactive', description='Installation mode (interactive, silent, or automated)')
-    variant('key',  default='',            description='The file installation key to use')
+    variant(
+        'mode',
+        default='interactive',
+        values=('interactive', 'silent', 'automated'),
+        description='Installation mode (interactive, silent, or automated)'
+    )
+
+    variant(
+        'key',
+        default='',
+        values=lambda x: True,  # Anything goes as a key
+        description='The file installation key to use'
+    )
 
     # Licensing
     license_required = True
diff --git a/var/spack/repos/builtin/packages/npb/package.py b/var/spack/repos/builtin/packages/npb/package.py
index abf14ef115..a6e428b344 100644
--- a/var/spack/repos/builtin/packages/npb/package.py
+++ b/var/spack/repos/builtin/packages/npb/package.py
@@ -22,9 +22,19 @@
 # License along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
+import numbers
+
 from spack import *
 
 
+def is_integral(x):
+    """Any integer value"""
+    try:
+        return isinstance(int(x), numbers.Integral) and not isinstance(x, bool)
+    except ValueError:
+        return False
+
+
 class Npb(MakefilePackage):
     """The NAS Parallel Benchmarks (NPB) are a small set of programs
     designed to help evaluate the performance of parallel supercomputers.
@@ -66,20 +76,39 @@ class Npb(MakefilePackage):
     )
 
     # TODO: Combine these into a single mutually exclusive variant
-    variant('mpi',    default=True,  description='Build the MPI implementation')
-    variant('openmp', default=False, description='Build the OpenMP implementation')
-    variant('serial', default=False, description='Build the serial version')
-
-    # TODO: Convert these to non-exclusive multi-valued variants
-    variant('names', default=','.join(valid_names),
-            description='Benchmark names (comma separated list)')
-    variant('classes', default=','.join(valid_classes),
-            description='Benchmark classes (comma separated list)')
+    variant(
+        'implementation',
+        default='mpi',
+        values=('serial', 'mpi', 'openmp'),
+        description='Selects one among the available implementations'
+    )
+
+    variant(
+        'names',
+        default=','.join(valid_names),
+        values=valid_names,
+        multi=True,
+        description='Benchmark names (comma separated list)'
+    )
+
+    variant(
+        'classes',
+        default=','.join(valid_classes),
+        values=valid_classes,
+        multi=True,
+        description='Benchmark classes (comma separated list)'
+    )
+
     # This variant only applies to the MPI implementation
-    variant('nprocs', default='1,2,4,8,16,32,64,128',
-            description='Number of processes (comma separated list)')
+    variant(
+        'nprocs',
+        default='1,2,4,8,16,32,64,128',
+        values=is_integral,
+        multi=True,
+        description='Number of processes (comma separated list)'
+    )
 
-    depends_on('mpi@2:', when='+mpi')
+    depends_on('mpi@2:', when='implementation=mpi')
 
     phases = ['edit', 'install']
 
@@ -88,11 +117,11 @@ class Npb(MakefilePackage):
 
     @property
     def build_directory(self):
-        if '+mpi' in self.spec:
+        if 'implementation=mpi' in self.spec:
             implementation = 'MPI'
-        elif '+openmp' in self.spec:
+        elif 'implementation=openmp' in self.spec:
             implementation = 'OMP'
-        elif '+serial' in self.spec:
+        elif 'implementation=serial' in self.spec:
             implementation = 'SER'
         else:
             raise RuntimeError('You must choose an implementation to build')
@@ -100,11 +129,11 @@ class Npb(MakefilePackage):
         return 'NPB{0}-{1}'.format(self.version.up_to(2), implementation)
 
     def edit(self, spec, prefix):
-        names   = spec.variants['names'].value.split(',')
-        classes = spec.variants['classes'].value.split(',')
-        nprocs  = spec.variants['nprocs'].value.split(',')
+        names   = spec.variants['names'].value
+        classes = spec.variants['classes'].value
+        nprocs  = spec.variants['nprocs'].value
 
-        if '+mpi' in spec:
+        if 'implementation=mpi' in spec:
             definitions = {
                 # Parallel Fortran
                 'MPIF77':     spec['mpi'].mpif77,
@@ -125,7 +154,7 @@ class Npb(MakefilePackage):
                 'BINDIR':     prefix.bin,
                 'RAND':       'randi8',
             }
-        elif '+openmp' in spec:
+        elif 'implementation=openmp' in spec:
             definitions = {
                 # Parallel Fortran
                 'F77':        spack_f77,
@@ -147,7 +176,7 @@ class Npb(MakefilePackage):
                 'RAND':       'randi8',
                 'WTIME':      'wtime.c',
             }
-        elif '+serial' in spec:
+        elif 'implementation=serial' in spec:
             definitions = {
                 # Parallel Fortran
                 'F77':        spack_f77,
@@ -187,7 +216,7 @@ class Npb(MakefilePackage):
                         if name == 'is' and classname == 'E':
                             continue
 
-                        if '+mpi' in spec:
+                        if 'implementation=mpi' in spec:
                             for nproc in nprocs:
                                 suite_def.write('{0}\t{1}\t{2}\n'.format(
                                     name, classname, nproc))
diff --git a/var/spack/repos/builtin/packages/plumed/package.py b/var/spack/repos/builtin/packages/plumed/package.py
index 3022cd7959..de59f57208 100644
--- a/var/spack/repos/builtin/packages/plumed/package.py
+++ b/var/spack/repos/builtin/packages/plumed/package.py
@@ -51,8 +51,12 @@ class Plumed(AutotoolsPackage):
     # The ones listed here are not built by default for various reasons,
     # such as stability, lack of testing, or lack of demand.
     # FIXME: This needs to be an optional
-    variant('optional_modules', default='all',
-            description='String that is used to build optional modules')
+    variant(
+        'optional_modules',
+        default='all',
+        values=lambda x: True,
+        description='String that is used to build optional modules'
+    )
     variant('shared', default=True, description='Builds shared libraries')
     variant('mpi', default=True, description='Activates MPI support')
     variant('gsl', default=True, description='Activates GSL support')
diff --git a/var/spack/repos/builtin/packages/texlive/package.py b/var/spack/repos/builtin/packages/texlive/package.py
index 70d5a1d789..3cfab6304d 100644
--- a/var/spack/repos/builtin/packages/texlive/package.py
+++ b/var/spack/repos/builtin/packages/texlive/package.py
@@ -36,8 +36,8 @@ class Texlive(Package):
     # update in synchrony.
     #
     # BEWARE: TexLive updates their installs frequently (probably why
-    # they call it *Live*...).  There is no good way to provide a 
-    # repeatable install of the package.  We try to keep up with the 
+    # they call it *Live*...).  There is no good way to provide a
+    # repeatable install of the package.  We try to keep up with the
     # digest values, but don't be surprised if this package is
     # briefly unbuildable.
     #
@@ -53,8 +53,12 @@ class Texlive(Package):
     #   minimal scheme (plain only)
     # See:
     # https://www.tug.org/texlive/doc/texlive-en/texlive-en.html#x1-25025r6
-    variant('scheme',  default="small",
-            description='Package subset to install (e.g. full, small, basic)')
+    variant(
+        'scheme',
+        default='small',
+        values=('minimal', 'basic', 'small', 'medium', 'full'),
+        description='Package subset to install'
+    )
 
     depends_on('perl', type='build')
 
@@ -62,7 +66,7 @@ class Texlive(Package):
         # Using texlive's mirror system leads to mysterious problems,
         # in lieu of being able to specify a repository as a variant, hardwire
         # a particular (slow, but central) one for now.
-        _repository='http://ctan.math.washington.edu/tex-archive/systems/texlive/tlnet/'
+        _repository = 'http://ctan.math.washington.edu/tex-archive/systems/texlive/tlnet/'
         env = os.environ
         env['TEXLIVE_INSTALL_PREFIX'] = prefix
         perl = which('perl')