Fix SPACK-40: Finish adding variant directive.

- Variants are now declarable in packages using the variant() directive.
- Variants are checked - you can't just ask for a random variant, it has to be declared.
- conditional logic (@when, if, '+debug' in spec, etc.) still required in package to
  implement variant.

3 files changed, 53 insertions, 14 deletions

diff --git a/lib/spack/spack/cmd/info.py b/lib/spack/spack/cmd/info.py
index eafafc601a..c6209523f0 100644
--- a/lib/spack/spack/cmd/info.py
+++ b/lib/spack/spack/cmd/info.py
@@ -22,12 +22,22 @@
 # along with this program; if not, write to the Free Software Foundation,
 # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
+import textwrap
 from llnl.util.tty.colify import *
 import spack
 import spack.fetch_strategy as fs
 
 description = "Get detailed information on a particular package"
 
+def padder(str_list, extra=0):
+    """Return a function to pad elements of a list."""
+    length = max(len(str(s)) for s in str_list) + extra
+    def pad(string):
+        string = str(string)
+        padding = max(0, length - len(string))
+        return string + (padding * ' ')
+    return pad
+
 
 def setup_parser(subparser):
     subparser.add_argument('name', metavar="PACKAGE", help="Name of package to get info for.")
@@ -42,13 +52,24 @@ def print_text_info(pkg):
     print "Safe versions:  "
 
     if not pkg.versions:
-        print("None.")
+        print("None")
     else:
-        maxlen = max(len(str(v)) for v in pkg.versions)
-        fmt = "%%-%ss" % maxlen
+        pad = padder(pkg.versions, 4)
         for v in reversed(sorted(pkg.versions)):
             f = fs.for_package_version(pkg, v)
-            print "    " + (fmt % v) + "    " + str(f)
+            print "    %s%s" % (pad(v), str(f))
+
+    print
+    print "Variants:"
+    if not pkg.variants:
+        print "None"
+    else:
+        pad = padder(pkg.variants, 4)
+        for name in sorted(pkg.variants):
+            v = pkg.variants[name]
+            print "    %s%s" % (
+                pad(('+' if v.default else '-') + name + ':'),
+                "\n".join(textwrap.wrap(v.description)))
 
     print
     print "Dependencies:"
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 2ae56fce33..5c17fe4044 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -58,6 +58,7 @@ import spack.error
 import spack.url
 from spack.version import Version
 from spack.patch import Patch
+from spack.variant import Variant
 from spack.spec import Spec, parse_anonymous_spec
 
 
@@ -240,17 +241,17 @@ def patch(pkg, url_or_filename, **kwargs):
 
 
 @directive(dicts='variants')
-def variant(pkg, name, description=""):
-    """Define a variant for the package.  Allows the user to supply
-       +variant/-variant in a spec.  You can optionally supply an
-       initial + or - to make the variant enabled or disabled by defaut.
-    """
-    if not re.match(r'^[-~+]?[A-Za-z0-9_][A-Za-z0-9_.-]*$', name):
-        raise DirectiveError("Invalid variant name in %s: '%s'"
-                             % (pkg.name, name))
+def variant(pkg, name, **kwargs):
+    """Define a variant for the package. Packager can specify a default
+    value (on or off) as well as a text description."""
+
+    default     = bool(kwargs.get('default', False))
+    description = str(kwargs.get('description', "")).strip()
+
+    if not re.match(spack.spec.identifier_re, name):
+        raise DirectiveError("Invalid variant name in %s: '%s'" % (pkg.name, name))
 
-    enabled = re.match(r'+', name)
-    pkg.variants[name] = enabled
+    pkg.variants[name] = Variant(default, description)
 
 
 class DirectiveError(spack.error.SpackError):
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index dffdccaddb..fca14f97db 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -110,6 +110,9 @@ from spack.util.string import *
 from spack.util.prefix import Prefix
 from spack.virtual import ProviderIndex
 
+# Valid pattern for an identifier in Spack
+identifier_re = r'\w[\w-]*'
+
 # Convenient names for color formats so that other things can use them
 compiler_color         = '@g'
 version_color          = '@c'
@@ -893,6 +896,11 @@ class Spec(object):
                 if not compilers.supported(spec.compiler):
                     raise UnsupportedCompilerError(spec.compiler.name)
 
+            # Ensure that variants all exist.
+            for vname, variant in spec.variants.items():
+                if vname not in spec.package.variants:
+                    raise UnknownVariantError(spec.name, vname)
+
 
     def constrain(self, other, **kwargs):
         other = self._autospec(other)
@@ -1354,6 +1362,8 @@ class SpecLexer(spack.parse.Lexer):
             (r'\~',        lambda scanner, val: self.token(OFF,   val)),
             (r'\%',        lambda scanner, val: self.token(PCT,   val)),
             (r'\=',        lambda scanner, val: self.token(EQ,    val)),
+            # This is more liberal than identifier_re (see above).
+            # Checked by check_identifier() for better error messages.
             (r'\w[\w.-]*', lambda scanner, val: self.token(ID,    val)),
             (r'\s+',       lambda scanner, val: None)])
 
@@ -1580,6 +1590,13 @@ class UnsupportedCompilerError(SpecError):
             "The '%s' compiler is not yet supported." % compiler_name)
 
 
+class UnknownVariantError(SpecError):
+    """Raised when the same variant occurs in a spec twice."""
+    def __init__(self, pkg, variant):
+        super(UnknownVariantError, self).__init__(
+            "Package %s has no variant %s!" % (pkg, variant))
+
+
 class DuplicateArchitectureError(SpecError):
     """Raised when the same architecture occurs in a spec twice."""
     def __init__(self, message):