Merge remote-tracking branch 'origin/features/variants' into features/optional-deps

16 files changed, 508 insertions, 304 deletions

diff --git a/lib/spack/docs/_themes/sphinx_rtd_theme/footer.html b/lib/spack/docs/_themes/sphinx_rtd_theme/footer.html
index 6347a440d7..d000dcbc2c 100644
--- a/lib/spack/docs/_themes/sphinx_rtd_theme/footer.html
+++ b/lib/spack/docs/_themes/sphinx_rtd_theme/footer.html
@@ -22,7 +22,12 @@
       {%- endif %}
     {%- endif %}
 
+    <br/>
+    Written by Todd Gamblin (<a href="mailto:tgamblin@llnl.gov">tgamblin@llnl.gov</a>) and
+    many contributors. LLNL-CODE-647188.
+
     {%- if last_updated %}
+      <br/>
       {% trans last_updated=last_updated|e %}Last updated on {{ last_updated }}.{% endtrans %}
     {%- endif %}
     </p>
@@ -33,4 +38,3 @@
   {%- endif %}
 
 </footer>
-
diff --git a/lib/spack/docs/conf.py b/lib/spack/docs/conf.py
index c2b2d0e37c..7303d7fef6 100644
--- a/lib/spack/docs/conf.py
+++ b/lib/spack/docs/conf.py
@@ -94,7 +94,7 @@ master_doc = 'index'
 
 # General information about the project.
 project = u'Spack'
-copyright = u'2013-2014, Lawrence Livermore National Laboratory'
+copyright = u'2013-2015, Lawrence Livermore National Laboratory.'
 
 # The version info for the project you're documenting, acts as replacement for
 # |version| and |release|, also used in various other places throughout the
@@ -203,7 +203,7 @@ html_last_updated_fmt = '%b %d, %Y'
 #html_show_sourcelink = True
 
 # If true, "Created using Sphinx" is shown in the HTML footer. Default is True.
-#html_show_sphinx = True
+#html_show_sphinx = False
 
 # If true, "(C) Copyright ..." is shown in the HTML footer. Default is True.
 #html_show_copyright = True
diff --git a/lib/spack/llnl/util/lang.py b/lib/spack/llnl/util/lang.py
index 332367f537..9e1bef18ca 100644
--- a/lib/spack/llnl/util/lang.py
+++ b/lib/spack/llnl/util/lang.py
@@ -126,22 +126,20 @@ def caller_locals():
         del stack
 
 
-def get_calling_package_name():
+def get_calling_module_name():
     """Make sure that the caller is a class definition, and return the
-       module's name.
+       enclosing module's name.
     """
     stack = inspect.stack()
     try:
-        # get calling function name (the relation)
-        relation = stack[1][3]
-
         # Make sure locals contain __module__
         caller_locals = stack[2][0].f_locals
     finally:
         del stack
 
     if not '__module__' in caller_locals:
-        raise ScopeError(relation)
+        raise RuntimeError("Must invoke get_calling_module_name() "
+                           "from inside a class definition!")
 
     module_name = caller_locals['__module__']
     base_name = module_name.split('.')[-1]
@@ -322,6 +320,24 @@ def match_predicate(*args):
     return match
 
 
+
+def DictWrapper(dictionary):
+    """Returns a class that wraps a dictionary and enables it to be used
+       like an object."""
+    class wrapper(object):
+        def __getattr__(self, name):        return dictionary[name]
+        def __setattr__(self, name, value): dictionary[name] = value
+        def setdefault(self, *args):        return dictionary.setdefault(*args)
+        def get(self, *args):               return dictionary.get(*args)
+        def keys(self):                     return dictionary.keys()
+        def values(self):                   return dictionary.values()
+        def items(self):                    return dictionary.items()
+        def __iter__(self):                 return iter(dictionary)
+
+
+    return wrapper()
+
+
 class RequiredAttributeError(ValueError):
     def __init__(self, message):
         super(RequiredAttributeError, self).__init__(message)
diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py
index eb891e3d57..053c4036d8 100644
--- a/lib/spack/spack/__init__.py
+++ b/lib/spack/spack/__init__.py
@@ -146,9 +146,9 @@ import llnl.util.filesystem
 from llnl.util.filesystem import *
 __all__ += llnl.util.filesystem.__all__
 
-import spack.relations
-from spack.relations import *
-__all__ += spack.relations.__all__
+import spack.directives
+from spack.directives import *
+__all__ += spack.directives.__all__
 
 import spack.util.executable
 from spack.util.executable import *
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/concretize.py b/lib/spack/spack/concretize.py
index 3f569f9dce..15e886ad3c 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -101,6 +101,16 @@ class DefaultConcretizer(object):
             spec.architecture = spack.architecture.sys_type()
 
 
+    def concretize_variants(self, spec):
+        """If the spec already has variants filled in, return.  Otherwise, add
+           the default variants from the package specification.
+        """
+        for name, variant in spec.package.variants.items():
+            if name not in spec.variants:
+                spec.variants[name] = spack.spec.VariantSpec(
+                    name, variant.default)
+
+
     def concretize_compiler(self, spec):
         """If the spec already has a compiler, we're done.  If not, then take
            the compiler used for the nearest ancestor with a compiler
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
new file mode 100644
index 0000000000..5c17fe4044
--- /dev/null
+++ b/lib/spack/spack/directives.py
@@ -0,0 +1,270 @@
+##############################################################################
+# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://scalability-llnl.github.io/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License (as published by
+# the Free Software Foundation) version 2.1 dated February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+"""This package contains directives that can be used within a package.
+
+Directives are functions that can be called inside a package
+definition to modify the package, for example:
+
+    class OpenMpi(Package):
+        depends_on("hwloc")
+        provides("mpi")
+        ...
+
+``provides`` and ``depends_on`` are spack directives.
+
+The available directives are:
+
+  * ``version``
+  * ``depends_on``
+  * ``provides``
+  * ``extends``
+  * ``patch``
+  * ``variant``
+
+"""
+__all__ = [ 'depends_on', 'extends', 'provides', 'patch', 'version',
+            'variant' ]
+
+import re
+import inspect
+
+from llnl.util.lang import *
+
+import spack
+import spack.spec
+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
+
+
+#
+# This is a list of all directives, built up as they are defined in
+# this file.
+#
+directives = {}
+
+
+def ensure_dicts(pkg):
+    """Ensure that a package has all the dicts required by directives."""
+    for name, d in directives.items():
+        d.ensure_dicts(pkg)
+
+
+class directive(object):
+    """Decorator for Spack directives.
+
+    Spack directives allow you to modify a package while it is being
+    defined, e.g. to add version or depenency information.  Directives
+    are one of the key pieces of Spack's package "langauge", which is
+    embedded in python.
+
+    Here's an example directive:
+
+        @directive(dicts='versions')
+        version(pkg, ...):
+            ...
+
+    This directive allows you write:
+
+        class Foo(Package):
+            version(...)
+
+    The ``@directive`` decorator handles a couple things for you:
+
+      1. Adds the class scope (pkg) as an initial parameter when
+         called, like a class method would.  This allows you to modify
+         a package from within a directive, while the package is still
+         being defined.
+
+      2. It automatically adds a dictionary called "versions" to the
+         package so that you can refer to pkg.versions.
+
+    The ``(dicts='versions')`` part ensures that ALL packages in Spack
+    will have a ``versions`` attribute after they're constructed, and
+    that if no directive actually modified it, it will just be an
+    empty dict.
+
+    This is just a modular way to add storage attributes to the
+    Package class, and it's how Spack gets information from the
+    packages to the core.
+
+    """
+
+    def __init__(self, **kwargs):
+        # dict argument allows directives to have storage on the package.
+        dicts = kwargs.get('dicts', None)
+
+        if isinstance(dicts, basestring):
+            dicts = (dicts,)
+        elif type(dicts) not in (list, tuple):
+            raise TypeError(
+                "dicts arg must be list, tuple, or string. Found %s."
+                % type(dicts))
+
+        self.dicts = dicts
+
+
+    def ensure_dicts(self, pkg):
+        """Ensure that a package has the dicts required by this directive."""
+        for d in self.dicts:
+            if not hasattr(pkg, d):
+                setattr(pkg, d, {})
+
+            attr = getattr(pkg, d)
+            if not isinstance(attr, dict):
+                raise spack.error.SpackError(
+                    "Package %s has non-dict %s attribute!" % (pkg, d))
+
+
+    def __call__(self, directive_function):
+        directives[directive_function.__name__] = self
+
+        def wrapped(*args, **kwargs):
+            pkg = DictWrapper(caller_locals())
+            self.ensure_dicts(pkg)
+
+            pkg.name = get_calling_module_name()
+            return directive_function(pkg, *args, **kwargs)
+
+        return wrapped
+
+
+@directive(dicts='versions')
+def version(pkg, ver, checksum=None, **kwargs):
+    """Adds a version and metadata describing how to fetch it.
+       Metadata is just stored as a dict in the package's versions
+       dictionary.  Package must turn it into a valid fetch strategy
+       later.
+    """
+    # special case checksum for backward compatibility
+    if checksum:
+        kwargs['md5'] = checksum
+
+    # Store kwargs for the package to later with a fetch_strategy.
+    pkg.versions[Version(ver)] = kwargs
+
+
+@directive(dicts='dependencies')
+def depends_on(pkg, *specs):
+    """Adds a dependencies local variable in the locals of
+       the calling class, based on args. """
+    for string in specs:
+        for spec in spack.spec.parse(string):
+            if pkg.name == spec.name:
+                raise CircularReferenceError('depends_on', pkg.name)
+            pkg.dependencies[spec.name] = spec
+
+
+@directive(dicts=('extendees', 'dependencies'))
+def extends(pkg, spec, **kwargs):
+    """Same as depends_on, but dependency is symlinked into parent prefix.
+
+    This is for Python and other language modules where the module
+    needs to be installed into the prefix of the Python installation.
+    Spack handles this by installing modules into their own prefix,
+    but allowing ONE module version to be symlinked into a parent
+    Python install at a time.
+
+    keyword arguments can be passed to extends() so that extension
+    packages can pass parameters to the extendee's extension
+    mechanism.
+
+    """
+    if pkg.extendees:
+        raise DirectiveError("Packages can extend at most one other package.")
+
+    spec = Spec(spec)
+    if pkg.name == spec.name:
+        raise CircularReferenceError('extends', pkg.name)
+    pkg.dependencies[spec.name] = spec
+    pkg.extendees[spec.name] = (spec, kwargs)
+
+
+@directive(dicts='provided')
+def provides(pkg, *specs, **kwargs):
+    """Allows packages to provide a virtual dependency.  If a package provides
+       'mpi', other packages can declare that they depend on "mpi", and spack
+       can use the providing package to satisfy the dependency.
+    """
+    spec_string = kwargs.get('when', pkg.name)
+    provider_spec = parse_anonymous_spec(spec_string, pkg.name)
+
+    for string in specs:
+        for provided_spec in spack.spec.parse(string):
+            if pkg.name == provided_spec.name:
+                raise CircularReferenceError('depends_on', pkg.name)
+            pkg.provided[provided_spec] = provider_spec
+
+
+@directive(dicts='patches')
+def patch(pkg, url_or_filename, **kwargs):
+    """Packages can declare patches to apply to source.  You can
+       optionally provide a when spec to indicate that a particular
+       patch should only be applied when the package's spec meets
+       certain conditions (e.g. a particular version).
+    """
+    level = kwargs.get('level', 1)
+    when  = kwargs.get('when', pkg.name)
+
+    when_spec = parse_anonymous_spec(when, pkg.name)
+    if when_spec not in pkg.patches:
+        pkg.patches[when_spec] = [Patch(pkg.name, url_or_filename, level)]
+    else:
+        # if this spec is identical to some other, then append this
+        # patch to the existing list.
+        pkg.patches[when_spec].append(Patch(pkg.name, url_or_filename, level))
+
+
+@directive(dicts='variants')
+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))
+
+    pkg.variants[name] = Variant(default, description)
+
+
+class DirectiveError(spack.error.SpackError):
+    """This is raised when something is wrong with a package directive."""
+    def __init__(self, directive, message):
+        super(DirectiveError, self).__init__(message)
+        self.directive = directive
+
+
+class CircularReferenceError(DirectiveError):
+    """This is raised when something depends on itself."""
+    def __init__(self, directive, package):
+        super(CircularReferenceError, self).__init__(
+            directive,
+            "Package '%s' cannot pass itself to %s." % (package, directive))
+        self.package = package
diff --git a/lib/spack/spack/multimethod.py b/lib/spack/spack/multimethod.py
index 974401e1aa..892619c6ac 100644
--- a/lib/spack/spack/multimethod.py
+++ b/lib/spack/spack/multimethod.py
@@ -195,7 +195,7 @@ class when(object):
     """
 class when(object):
     def __init__(self, spec):
-        pkg = get_calling_package_name()
+        pkg = get_calling_module_name()
         self.spec = parse_anonymous_spec(spec, pkg)
 
     def __call__(self, method):
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 7d9eca5077..2891791339 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -55,6 +55,7 @@ import spack.error
 import spack.compilers
 import spack.mirror
 import spack.hooks
+import spack.directives
 import spack.build_environment as build_env
 import spack.url as url
 import spack.fetch_strategy as fs
@@ -301,32 +302,6 @@ class Package(object):
     clean() (some of them do this), and others to provide custom behavior.
 
     """
-
-    #
-    # These variables are defaults for the various "relations".
-    #
-    """Map of information about Versions of this package.
-       Map goes: Version -> dict of attributes"""
-    versions = {}
-
-    """Specs of dependency packages, keyed by name."""
-    dependencies = {}
-
-    """Specs of virtual packages provided by this package, keyed by name."""
-    provided = {}
-
-    """Specs of conflicting packages, keyed by name. """
-    conflicted = {}
-
-    """Patches to apply to newly expanded source, if any."""
-    patches = {}
-
-    """Specs of package this one extends, or None.
-
-    Currently, ppackages can extend at most one other package.
-    """
-    extendees = {}
-
     #
     # These are default values for instance variables.
     #
@@ -350,20 +325,8 @@ class Package(object):
         if '.' in self.name:
             self.name = self.name[self.name.rindex('.') + 1:]
 
-        # Sanity check some required variables that could be
-        # overridden by package authors.
-        def ensure_has_dict(attr_name):
-            if not hasattr(self, attr_name):
-                raise PackageError("Package %s must define %s" % attr_name)
-
-            attr = getattr(self, attr_name)
-            if not isinstance(attr, dict):
-                raise PackageError("Package %s has non-dict %s attribute!"
-                                   % (self.name, attr_name))
-        ensure_has_dict('versions')
-        ensure_has_dict('dependencies')
-        ensure_has_dict('conflicted')
-        ensure_has_dict('patches')
+        # Sanity check attributes required by Spack directives.
+        spack.directives.ensure_dicts(type(self))
 
         # Check versions in the versions dict.
         for v in self.versions:
diff --git a/lib/spack/spack/relations.py b/lib/spack/spack/relations.py
deleted file mode 100644
index a0c7723473..0000000000
--- a/lib/spack/spack/relations.py
+++ /dev/null
@@ -1,215 +0,0 @@
-##############################################################################
-# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
-# Produced at the Lawrence Livermore National Laboratory.
-#
-# This file is part of Spack.
-# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
-# LLNL-CODE-647188
-#
-# For details, see https://scalability-llnl.github.io/spack
-# Please also see the LICENSE file for our notice and the LGPL.
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License (as published by
-# the Free Software Foundation) version 2.1 dated February 1999.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
-# conditions of the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU Lesser General Public License
-# along with this program; if not, write to the Free Software Foundation,
-# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
-##############################################################################
-"""
-This package contains relationships that can be defined among packages.
-Relations are functions that can be called inside a package definition,
-for example:
-
-    class OpenMPI(Package):
-        depends_on("hwloc")
-        provides("mpi")
-        ...
-
-The available relations are:
-
-depends_on
-    Above, the OpenMPI package declares that it "depends on" hwloc.  This means
-    that the hwloc package needs to be installed before OpenMPI can be
-    installed.  When a user runs 'spack install openmpi', spack will fetch
-    hwloc and install it first.
-
-provides
-    This is useful when more than one package can satisfy a dependence.  Above,
-    OpenMPI declares that it "provides" mpi.  Other implementations of the MPI
-    interface, like mvapich and mpich, also provide mpi, e.g.:
-
-        class Mvapich(Package):
-            provides("mpi")
-            ...
-
-        class Mpich(Package):
-            provides("mpi")
-            ...
-
-    Instead of depending on openmpi, mvapich, or mpich, another package can
-    declare that it depends on "mpi":
-
-        class Mpileaks(Package):
-            depends_on("mpi")
-            ...
-
-    Now the user can pick which MPI they would like to build with when they
-    install mpileaks.  For example, the user could install 3 instances of
-    mpileaks, one for each MPI version, by issuing these three commands:
-
-        spack install mpileaks ^openmpi
-        spack install mpileaks ^mvapich
-        spack install mpileaks ^mpich
-"""
-__all__ = [ 'depends_on', 'extends', 'provides', 'patch', 'version' ]
-
-import re
-import inspect
-
-from llnl.util.lang import *
-
-import spack
-import spack.spec
-import spack.error
-import spack.url
-from spack.version import Version
-from spack.patch import Patch
-from spack.spec import Spec, parse_anonymous_spec
-
-
-
-def version(ver, checksum=None, **kwargs):
-    """Adds a version and metadata describing how to fetch it.
-       Metadata is just stored as a dict in the package's versions
-       dictionary.  Package must turn it into a valid fetch strategy
-       later.
-    """
-    pkg = caller_locals()
-    versions = pkg.setdefault('versions', {})
-
-    # special case checksum for backward compatibility
-    if checksum:
-        kwargs['md5'] = checksum
-
-    # Store the kwargs for the package to use later when constructing
-    # a fetch strategy.
-    versions[Version(ver)] = kwargs
-
-
-def depends_on(*specs):
-    """Adds a dependencies local variable in the locals of
-       the calling class, based on args. """
-    pkg = get_calling_package_name()
-    clocals = caller_locals()
-    dependencies = clocals.setdefault('dependencies', {})
-
-    for string in specs:
-        for spec in spack.spec.parse(string):
-            if pkg == spec.name:
-                raise CircularReferenceError('depends_on', pkg)
-            dependencies[spec.name] = spec
-
-
-def extends(spec, **kwargs):
-    """Same as depends_on, but dependency is symlinked into parent prefix.
-
-    This is for Python and other language modules where the module
-    needs to be installed into the prefix of the Python installation.
-    Spack handles this by installing modules into their own prefix,
-    but allowing ONE module version to be symlinked into a parent
-    Python install at a time.
-
-    keyword arguments can be passed to extends() so that extension
-    packages can pass parameters to the extendee's extension
-    mechanism.
-
-    """
-    pkg = get_calling_package_name()
-    clocals = caller_locals()
-    dependencies = clocals.setdefault('dependencies', {})
-    extendees = clocals.setdefault('extendees', {})
-    if extendees:
-        raise RelationError("Packages can extend at most one other package.")
-
-    spec = Spec(spec)
-    if pkg == spec.name:
-        raise CircularReferenceError('extends', pkg)
-    dependencies[spec.name] = spec
-    extendees[spec.name] = (spec, kwargs)
-
-
-def provides(*specs, **kwargs):
-    """Allows packages to provide a virtual dependency.  If a package provides
-       'mpi', other packages can declare that they depend on "mpi", and spack
-       can use the providing package to satisfy the dependency.
-    """
-    pkg = get_calling_package_name()
-    spec_string = kwargs.get('when', pkg)
-    provider_spec = parse_anonymous_spec(spec_string, pkg)
-
-    provided = caller_locals().setdefault("provided", {})
-    for string in specs:
-        for provided_spec in spack.spec.parse(string):
-            if pkg == provided_spec.name:
-                raise CircularReferenceError('depends_on', pkg)
-            provided[provided_spec] = provider_spec
-
-
-def patch(url_or_filename, **kwargs):
-    """Packages can declare patches to apply to source.  You can
-       optionally provide a when spec to indicate that a particular
-       patch should only be applied when the package's spec meets
-       certain conditions (e.g. a particular version).
-    """
-    pkg = get_calling_package_name()
-    level = kwargs.get('level', 1)
-    when_spec = parse_anonymous_spec(kwargs.get('when', pkg), pkg)
-
-    patches = caller_locals().setdefault('patches', {})
-    if when_spec not in patches:
-        patches[when_spec] = [Patch(pkg, url_or_filename, level)]
-    else:
-        # if this spec is identical to some other, then append this
-        # patch to the existing list.
-        patches[when_spec].append(Patch(pkg, url_or_filename, level))
-
-
-def conflicts(*specs):
-    """Packages can declare conflicts with other packages.
-       This can be as specific as you like: use regular spec syntax.
-
-       NOT YET IMPLEMENTED.
-    """
-    # TODO: implement conflicts
-    pass
-
-
-class RelationError(spack.error.SpackError):
-    """This is raised when something is wrong with a package relation."""
-    def __init__(self, relation, message):
-        super(RelationError, self).__init__(message)
-        self.relation = relation
-
-
-class ScopeError(RelationError):
-    """This is raised when a relation is called from outside a spack package."""
-    def __init__(self, relation):
-        super(ScopeError, self).__init__(
-            relation,
-            "Must invoke '%s' from inside a class definition!" % relation)
-
-
-class CircularReferenceError(RelationError):
-    """This is raised when something depends on itself."""
-    def __init__(self, relation, package):
-        super(CircularReferenceError, self).__init__(
-            relation,
-            "Package '%s' cannot pass itself to %s." % (package, relation))
-        self.package = package
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index dffdccaddb..7eb9d42cd1 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'
@@ -267,7 +270,7 @@ class CompilerSpec(object):
 
 
 @key_ordering
-class Variant(object):
+class VariantSpec(object):
     """Variants are named, build-time options for a package.  Names depend
        on the particular package being built, and each named variant can
        be enabled or disabled.
@@ -282,7 +285,7 @@ class Variant(object):
 
 
     def copy(self):
-        return Variant(self.name, self.enabled)
+        return VariantSpec(self.name, self.enabled)
 
 
     def __str__(self):
@@ -291,9 +294,44 @@ class Variant(object):
 
 
 class VariantMap(HashableMap):
+    def __init__(self, spec):
+        super(VariantMap, self).__init__()
+        self.spec = spec
+
+
     def satisfies(self, other):
-        return all(self[key].enabled == other[key].enabled
-                   for key in other if key in self)
+        if self.spec._concrete:
+            return all(k in self and self[k].enabled == other[k].enabled
+                       for k in other)
+        else:
+            return all(self[k].enabled == other[k].enabled
+                       for k in other if k in self)
+
+
+    def constrain(self, other):
+        if other.spec._concrete:
+            for k in self:
+                if k not in other:
+                    raise UnsatisfiableVariantSpecError(self[k], '<absent>')
+
+        for k in other:
+            if k in self:
+                if self[k].enabled != other[k].enabled:
+                    raise UnsatisfiableVariantSpecError(self[k], other[k])
+            else:
+                self[k] = other[k].copy()
+
+    @property
+    def concrete(self):
+        return self.spec._concrete or all(
+            v in self for v in self.spec.package.variants)
+
+
+    def copy(self):
+        clone = VariantMap(None)
+        for name, variant in self.items():
+            clone[name] = variant.copy()
+        return clone
 
 
     def __str__(self):
@@ -340,10 +378,11 @@ class Spec(object):
         self.name = other.name
         self.dependents = other.dependents
         self.versions = other.versions
-        self.variants = other.variants
         self.architecture = other.architecture
         self.compiler = other.compiler
         self.dependencies = other.dependencies
+        self.variants = other.variants
+        self.variants.spec = self
 
         # Specs are by default not assumed to be normal, but in some
         # cases we've read them from a file want to assume normal.
@@ -372,7 +411,7 @@ class Spec(object):
         """Called by the parser to add a variant."""
         if name in self.variants: raise DuplicateVariantError(
                 "Cannot specify variant '%s' twice" % name)
-        self.variants[name] = Variant(name, enabled)
+        self.variants[name] = VariantSpec(name, enabled)
 
 
     def _set_compiler(self, compiler):
@@ -436,14 +475,15 @@ class Spec(object):
     @property
     def concrete(self):
         """A spec is concrete if it can describe only ONE build of a package.
-           If any of the name, version, architecture, compiler, or depdenencies
-           are ambiguous,then it is not concrete.
+           If any of the name, version, architecture, compiler,
+           variants, or depdenencies are ambiguous,then it is not concrete.
         """
         if self._concrete:
             return True
 
         self._concrete = bool(not self.virtual
                               and self.versions.concrete
+                              and self.variants.concrete
                               and self.architecture
                               and self.compiler and self.compiler.concrete
                               and self.dependencies.concrete)
@@ -604,6 +644,7 @@ class Spec(object):
                 spack.concretizer.concretize_architecture(self)
                 spack.concretizer.concretize_compiler(self)
                 spack.concretizer.concretize_version(self)
+                spack.concretizer.concretize_variants(self)
             presets[self.name] = self
 
         visited.add(self.name)
@@ -786,8 +827,7 @@ class Spec(object):
                     else:
                         required = index.providers_for(vspec.name)
                         if required:
-                            raise UnsatisfiableProviderSpecError(
-                                required[0], pkg_dep)
+                            raise UnsatisfiableProviderSpecError(required[0], pkg_dep)
                 provider_index.update(pkg_dep)
 
             if name not in spec_deps:
@@ -893,6 +933,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)
@@ -921,7 +966,7 @@ class Spec(object):
             self.compiler = other.compiler
 
         self.versions.intersect(other.versions)
-        self.variants.update(other.variants)
+        self.variants.constrain(other.variants)
         self.architecture = self.architecture or other.architecture
 
         if constrain_deps:
@@ -990,11 +1035,13 @@ class Spec(object):
         # All these attrs have satisfies criteria of their own,
         # but can be None to indicate no constraints.
         for s, o in ((self.versions, other.versions),
-                     (self.variants, other.variants),
                      (self.compiler, other.compiler)):
             if s and o and not s.satisfies(o):
                 return False
 
+        if not self.variants.satisfies(other.variants):
+            return False
+
         # Architecture satisfaction is currently just string equality.
         # Can be None for unconstrained, though.
         if (self.architecture and other.architecture and
@@ -1061,11 +1108,12 @@ class Spec(object):
         # Local node attributes get copied first.
         self.name = other.name
         self.versions = other.versions.copy()
-        self.variants = other.variants.copy()
         self.architecture = other.architecture
         self.compiler = other.compiler.copy() if other.compiler else None
         self.dependents = DependencyMap()
         self.dependencies = DependencyMap()
+        self.variants = other.variants.copy()
+        self.variants.spec = self
 
         # If we copy dependencies, preserve DAG structure in the new spec
         if kwargs.get('deps', True):
@@ -1354,6 +1402,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)])
 
@@ -1399,7 +1449,7 @@ class SpecParser(spack.parse.Parser):
         spec = Spec.__new__(Spec)
         spec.name = self.token.value
         spec.versions = VersionList()
-        spec.variants = VariantMap()
+        spec.variants = VariantMap(spec)
         spec.architecture = None
         spec.compiler = None
         spec.dependents   = DependencyMap()
@@ -1580,6 +1630,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):
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index a7f4812c8c..cc839a2340 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -35,7 +35,13 @@ class ConcretizeTest(MockPackagesTest):
             self.assertEqual(abstract.versions, concrete.versions)
 
         if abstract.variants:
-            self.assertEqual(abstract.versions, concrete.versions)
+            for name in abstract.variants:
+                avariant = abstract.variants[name]
+                cvariant = concrete.variants[name]
+                self.assertEqual(avariant.enabled, cvariant.enabled)
+
+        for name in abstract.package.variants:
+            self.assertTrue(name in concrete.variants)
 
         if abstract.compiler and abstract.compiler.concrete:
             self.assertEqual(abstract.compiler, concrete.compiler)
@@ -66,6 +72,12 @@ class ConcretizeTest(MockPackagesTest):
         self.check_concretize('libelf')
 
 
+    def test_concretize_variant(self):
+        self.check_concretize('mpich+debug')
+        self.check_concretize('mpich~debug')
+        self.check_concretize('mpich')
+
+
     def test_concretize_with_virtual(self):
         self.check_concretize('mpileaks ^mpi')
         self.check_concretize('mpileaks ^mpi@:1.1')
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index fb67aa8a8d..ecbc46981c 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -242,12 +242,6 @@ class SpecDagTest(MockPackagesTest):
         self.assertRaises(spack.spec.UnsatisfiableCompilerSpecError, spec.normalize)
 
 
-    def test_unsatisfiable_variant(self):
-        set_pkg_dep('mpileaks', 'mpich+debug')
-        spec = Spec('mpileaks ^mpich~debug ^callpath ^dyninst ^libelf ^libdwarf')
-        self.assertRaises(spack.spec.UnsatisfiableVariantSpecError, spec.normalize)
-
-
     def test_unsatisfiable_architecture(self):
         set_pkg_dep('mpileaks', 'mpich=bgqos_0')
         spec = Spec('mpileaks ^mpich=sles_10_ppc64 ^callpath ^dyninst ^libelf ^libdwarf')
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index 5fb09e68af..8614b74c7a 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -33,8 +33,8 @@ class SpecSematicsTest(MockPackagesTest):
     # ================================================================================
     # Utility functions to set everything up.
     # ================================================================================
-    def check_satisfies(self, spec, anon_spec):
-        left = Spec(spec)
+    def check_satisfies(self, spec, anon_spec, concrete=False):
+        left = Spec(spec, concrete=concrete)
         right = parse_anonymous_spec(anon_spec, left.name)
 
         # Satisfies is one-directional.
@@ -46,8 +46,8 @@ class SpecSematicsTest(MockPackagesTest):
         right.copy().constrain(left)
 
 
-    def check_unsatisfiable(self, spec, anon_spec):
-        left = Spec(spec)
+    def check_unsatisfiable(self, spec, anon_spec, concrete=False):
+        left = Spec(spec, concrete=concrete)
         right = parse_anonymous_spec(anon_spec, left.name)
 
         self.assertFalse(left.satisfies(right))
@@ -71,7 +71,7 @@ class SpecSematicsTest(MockPackagesTest):
 
 
     # ================================================================================
-    # Satisfiability and constraints
+    # Satisfiability
     # ================================================================================
     def test_satisfies(self):
         self.check_satisfies('libelf@0.8.13', '@0:1')
@@ -96,6 +96,9 @@ class SpecSematicsTest(MockPackagesTest):
         self.check_unsatisfiable('foo@4.0%pgi', '@1:3%pgi')
         self.check_unsatisfiable('foo@4.0%pgi@4.5', '@1:3%pgi@4.4:4.6')
 
+        self.check_satisfies('foo %gcc@4.7.3', '%gcc@4.7')
+        self.check_unsatisfiable('foo %gcc@4.7', '%gcc@4.7.3')
+
 
     def test_satisfies_architecture(self):
         self.check_satisfies('foo=chaos_5_x86_64_ib', '=chaos_5_x86_64_ib')
@@ -147,7 +150,40 @@ class SpecSematicsTest(MockPackagesTest):
         self.check_unsatisfiable('mpileaks^mpi@3:', '^mpich@1.0')
 
 
-    def test_constrain(self):
+    def test_satisfies_matching_variant(self):
+        self.check_satisfies('mpich+foo', 'mpich+foo')
+        self.check_satisfies('mpich~foo', 'mpich~foo')
+
+
+    def test_satisfies_unconstrained_variant(self):
+        # only asked for mpich, no constraints.  Either will do.
+        self.check_satisfies('mpich+foo', 'mpich')
+        self.check_satisfies('mpich~foo', 'mpich')
+
+
+    def test_unsatisfiable_variants(self):
+        # This case is different depending on whether the specs are concrete.
+
+        # 'mpich' is not concrete:
+        self.check_satisfies('mpich', 'mpich+foo', False)
+        self.check_satisfies('mpich', 'mpich~foo', False)
+
+        # 'mpich' is concrete:
+        self.check_unsatisfiable('mpich', 'mpich+foo', True)
+        self.check_unsatisfiable('mpich', 'mpich~foo', True)
+
+
+    def test_unsatisfiable_variant_mismatch(self):
+        # No matchi in specs
+        self.check_unsatisfiable('mpich~foo', 'mpich+foo')
+        self.check_unsatisfiable('mpich+foo', 'mpich~foo')
+
+
+
+    # ================================================================================
+    # Constraints
+    # ================================================================================
+    def test_constrain_variants(self):
         self.check_constrain('libelf@2.1:2.5', 'libelf@0:2.5', 'libelf@2.1:3')
         self.check_constrain('libelf@2.1:2.5%gcc@4.5:4.6',
                              'libelf@0:2.5%gcc@2:4.6', 'libelf@2.1:3%gcc@4.5:4.7')
@@ -158,6 +194,8 @@ class SpecSematicsTest(MockPackagesTest):
         self.check_constrain('libelf+debug~foo', 'libelf+debug', 'libelf~foo')
         self.check_constrain('libelf+debug~foo', 'libelf+debug', 'libelf+debug~foo')
 
+
+    def test_constrain_arch(self):
         self.check_constrain('libelf=bgqos_0', 'libelf=bgqos_0', 'libelf=bgqos_0')
         self.check_constrain('libelf=bgqos_0', 'libelf', 'libelf=bgqos_0')
 
@@ -170,8 +208,3 @@ class SpecSematicsTest(MockPackagesTest):
         self.check_invalid_constraint('libelf+debug~foo', 'libelf+debug+foo')
 
         self.check_invalid_constraint('libelf=bgqos_0', 'libelf=x86_54')
-
-
-    def test_compiler_satisfies(self):
-        self.check_satisfies('foo %gcc@4.7.3', '%gcc@4.7')
-        self.check_unsatisfiable('foo %gcc@4.7', '%gcc@4.7.3')
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
new file mode 100644
index 0000000000..3d3e2b0f6d
--- /dev/null
+++ b/lib/spack/spack/variant.py
@@ -0,0 +1,36 @@
+##############################################################################
+# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://scalability-llnl.github.io/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License (as published by
+# the Free Software Foundation) version 2.1 dated February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+"""Variant is a class describing flags on builds, or "variants".
+
+Could be generalized later to describe aribitrary parameters, but
+currently variants are just flags.
+
+"""
+
+class Variant(object):
+    """Represents a variant on a build.  Can be either on or off."""
+    def __init__(self, default, description):
+        self.default     = bool(default)
+        self.description = str(description)
diff --git a/var/spack/mock_packages/mpich/package.py b/var/spack/mock_packages/mpich/package.py
index 75a939a892..f77d3efc5d 100644
--- a/var/spack/mock_packages/mpich/package.py
+++ b/var/spack/mock_packages/mpich/package.py
@@ -30,6 +30,9 @@ class Mpich(Package):
     list_url   = "http://www.mpich.org/static/downloads/"
     list_depth = 2
 
+    variant('debug', default=False,
+            description="Compile MPICH with debug flags.")
+
     version('3.0.4', '9c5d5d4fe1e17dd12153f40bc5b6dbc0')
     version('3.0.3', 'foobarbaz')
     version('3.0.2', 'foobarbaz')