From fcb4dfc3077d2cb96c0fe84b6615581640637e3f Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Sat, 19 Sep 2020 07:54:26 +0200
Subject: Ensure variant defaults are parsable from CLI. (#18661)

- Add a unit test to check if there are unparsable defaults
- Fix 'rust' and 'nsimd' variants
---
 lib/spack/spack/test/package_sanity.py            | 18 +++++++++++++++-
 var/spack/repos/builtin/packages/nsimd/package.py | 25 ++++++++++-------------
 var/spack/repos/builtin/packages/rust/package.py  | 11 +++++-----
 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index 25494e1ab2..2496bc99b7 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -136,7 +136,7 @@ def test_all_packages_use_sha256_checksums():
                 if bad_digest:
                     errors.append(
                         "All packages must use sha256 checksums."
-                        "Resource in %s uses %s." % (name, v, bad_digest)
+                        "Resource in %s uses %s." % (name, bad_digest)
                     )
 
     assert [] == errors
@@ -203,3 +203,19 @@ def test_all_dependencies_exist():
     assert not missing, "These packages have missing dependencies:\n" + (
         "\n".join(lines)
     )
+
+
+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():
+            default_is_parsable = (
+                # Permitting a default that is an instance on 'int' permits
+                # to have foo=false or foo=0. Other falsish values are
+                # not allowed, since they can't be parsed from cli ('foo=')
+                isinstance(variant.default, int) or variant.default
+            )
+            if not default_is_parsable:
+                failing.append((pkg.name, variant_name))
+    assert not failing
diff --git a/var/spack/repos/builtin/packages/nsimd/package.py b/var/spack/repos/builtin/packages/nsimd/package.py
index 1d22be943e..f9706f834d 100644
--- a/var/spack/repos/builtin/packages/nsimd/package.py
+++ b/var/spack/repos/builtin/packages/nsimd/package.py
@@ -2,10 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-from spack import *
-
-
 class Nsimd(CMakePackage):
     """NSIMD is a vectorization library that abstracts SIMD programming.
     It was designed to exploit the maximum power of processors
@@ -28,11 +24,8 @@ class Nsimd(CMakePackage):
                 'NEON128', 'AARCH64', 'SVE',
             ),
             multi=False)
-    variant('optionals',
-            default=(),
-            description='Optional SIMD features',
-            values=('FMA', 'FP16'),
-            multi=True)
+    variant('optionals', values=any_combination_of('FMA', 'FP16'),
+            description='Optional SIMD features',)
 
     conflicts('simd=none', msg="SIMD instruction set not defined")
 
@@ -56,10 +49,14 @@ class Nsimd(CMakePackage):
         python(*options)
 
     def cmake_args(self):
+        # Required SIMD argument
         simd = self.spec.variants['simd'].value
-        optionals = ';'.join(self.spec.variants['optionals'].value)
-        cmake_args = [
-            "-DSIMD={0}".format(simd),
-            "-DSIMD_OPTIONALS={0}".format(optionals),
-        ]
+        cmake_args = ["-DSIMD={0}".format(simd)]
+
+        # Optional SIMD instructions to be turned on explicitly
+        optionals_value = self.spec.variants['optionals'].value
+        if optionals_value != 'none':
+            optionals_arg = ';'.join(optionals_value)
+            cmake_args.append("-DSIMD_OPTIONALS={0}".format(optionals_arg))
+
         return cmake_args
diff --git a/var/spack/repos/builtin/packages/rust/package.py b/var/spack/repos/builtin/packages/rust/package.py
index abfd4e8e0b..e61e962013 100644
--- a/var/spack/repos/builtin/packages/rust/package.py
+++ b/var/spack/repos/builtin/packages/rust/package.py
@@ -2,8 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-from spack import *
 from six import iteritems
 
 
@@ -56,9 +54,7 @@ class Rust(Package):
         description='Install Rust source files'
     )
     variant(
-        'extra_targets',
-        default=(),
-        multi=True,
+        'extra_targets', default='none', multi=True,
         description='Triples for extra targets to enable. For supported targets, see: https://doc.rust-lang.org/nightly/rustc/platform-support.html'
     )
 
@@ -502,7 +498,10 @@ class Rust(Package):
 
         ar = which('ar', required=True)
 
-        extra_targets = list(self.spec.variants['extra_targets'].value)
+        extra_targets = []
+        if self.spec.variants['extra_targets'].value != 'none':
+            extra_targets = list(self.spec.variants['extra_targets'].value)
+
         targets = [self.get_rust_target()] + extra_targets
         target_spec = 'target=[' + \
             ','.join('"{0}"'.format(target) for target in targets) + ']'
-- 
cgit v1.2.3-60-g2f50