Directive inheritance: laziness for the win (#2623)

* inheritance of directives: using meta-classes to inject attributes coming from directives into packages + lazy directives

* _dep_types -> dependency_types
* using a meta-class to inject directives into packages
* directives are lazy

fixes #2466

* directives.py: allows for multiple inheritance. Added blank lines as suggested by @tgamblin

* directives.py: added a test for simple inheritance of directives

* Minor improvement requested by @tgamblin

CMakePackage: importing names from spack.directives
directives: wrap __new__ to respect pep8

* Refactoring requested by @tgamblin

directives: removed global variables in favor of class variables. Simplified the interface for directives (they return a callable on a package or a list of them).

7 files changed, 257 insertions, 197 deletions

diff --git a/lib/spack/llnl/util/lang.py b/lib/spack/llnl/util/lang.py
index 253334c416..637d18cd63 100644
--- a/lib/spack/llnl/util/lang.py
+++ b/lib/spack/llnl/util/lang.py
@@ -342,38 +342,6 @@ 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()
-
-
 def dedupe(sequence):
     """Yields a stable de-duplication of an hashable sequence
 
diff --git a/lib/spack/spack/build_systems/cmake.py b/lib/spack/spack/build_systems/cmake.py
index cb1076d7b7..e097f7e44f 100644
--- a/lib/spack/spack/build_systems/cmake.py
+++ b/lib/spack/spack/build_systems/cmake.py
@@ -30,6 +30,7 @@ import platform
 import llnl.util.tty as tty
 import spack.build_environment
 from llnl.util.filesystem import working_dir, join_path
+from spack.directives import depends_on
 from spack.package import PackageBase
 
 
@@ -49,6 +50,8 @@ class CMakePackage(PackageBase):
     # build-system class we are using
     build_system_class = 'CMakePackage'
 
+    depends_on('cmake', type='build')
+
     def build_type(self):
         """Override to provide the correct build_type in case a complex
         logic is needed
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 9cf00334a8..d499e96513 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -46,129 +46,181 @@ The available directives are:
 
 """
 
-import re
-import os.path
+import collections
 import functools
+import inspect
+import os.path
+import re
 
-from llnl.util.lang import *
-from llnl.util.filesystem import join_path
-
+import llnl.util.lang
 import spack
-import spack.spec
 import spack.error
+import spack.spec
 import spack.url
-from spack.version import Version
+from llnl.util.filesystem import join_path
+from spack.fetch_strategy import from_kwargs
 from spack.patch import Patch
-from spack.variant import Variant
-from spack.spec import Spec, parse_anonymous_spec
 from spack.resource import Resource
-from spack.fetch_strategy import from_kwargs
-
-__all__ = ['depends_on', 'extends', 'provides', 'patch', 'version', 'variant',
-           'resource']
-
-#
-# 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 dependency information.  Directives
-    are one of the key pieces of Spack's package "language", 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.
+from spack.spec import Spec, parse_anonymous_spec
+from spack.variant import Variant
+from spack.version import Version
 
-    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.
+__all__ = []
 
-    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.
 
+class DirectiveMetaMixin(type):
+    """Flushes the directives that were temporarily stored in the staging
+    area into the package.
     """
 
-    def __init__(self, dicts=None):
+    # Set of all known directives
+    _directive_names = set()
+    _directives_to_be_executed = []
+
+    def __new__(mcs, name, bases, attr_dict):
+        # Initialize the attribute containing the list of directives
+        # to be executed. Here we go reversed because we want to execute
+        # commands:
+        # 1. in the order they were defined
+        # 2. following the MRO
+        attr_dict['_directives_to_be_executed'] = []
+        for base in reversed(bases):
+            try:
+                directive_from_base = base._directives_to_be_executed
+                attr_dict['_directives_to_be_executed'].extend(
+                    directive_from_base
+                )
+            except AttributeError:
+                # The base class didn't have the required attribute.
+                # Continue searching
+                pass
+        # De-duplicates directives from base classes
+        attr_dict['_directives_to_be_executed'] = [
+            x for x in llnl.util.lang.dedupe(
+                attr_dict['_directives_to_be_executed']
+            )
+        ]
+
+        # Move things to be executed from module scope (where they
+        # are collected first) to class scope
+        if DirectiveMetaMixin._directives_to_be_executed:
+            attr_dict['_directives_to_be_executed'].extend(
+                DirectiveMetaMixin._directives_to_be_executed
+            )
+            DirectiveMetaMixin._directives_to_be_executed = []
+
+        return super(DirectiveMetaMixin, mcs).__new__(
+            mcs, name, bases, attr_dict
+        )
+
+    def __init__(cls, name, bases, attr_dict):
+        # The class is being created: if it is a package we must ensure
+        # that the directives are called on the class to set it up
+        module = inspect.getmodule(cls)
+        if 'spack.pkg' in module.__name__:
+            # Package name as taken
+            # from llnl.util.lang.get_calling_module_name
+            pkg_name = module.__name__.split('.')[-1]
+            setattr(cls, 'name', pkg_name)
+            # Ensure the presence of the dictionaries associated
+            # with the directives
+            for d in DirectiveMetaMixin._directive_names:
+                setattr(cls, d, {})
+            # Lazy execution of directives
+            for command in cls._directives_to_be_executed:
+                command(cls)
+
+        super(DirectiveMetaMixin, cls).__init__(name, bases, attr_dict)
+
+    @staticmethod
+    def directive(dicts=None):
+        """Decorator for Spack directives.
+
+        Spack directives allow you to modify a package while it is being
+        defined, e.g. to add version or dependency information.  Directives
+        are one of the key pieces of Spack's package "language", 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.
+
+        """
+        global __all__
+
         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))
+        if not isinstance(dicts, collections.Sequence):
+            message = "dicts arg must be list, tuple, or string. Found {0}"
+            raise TypeError(message.format(type(dicts)))
+        # Add the dictionary names if not already there
+        DirectiveMetaMixin._directive_names |= set(dicts)
 
-        self.dicts = dicts
+        # This decorator just returns the directive functions
+        def _decorator(decorated_function):
+            __all__.append(decorated_function.__name__)
 
-    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, {})
+            @functools.wraps(decorated_function)
+            def _wrapper(*args, **kwargs):
+                # A directive returns either something that is callable on a
+                # package or a sequence of them
+                values = decorated_function(*args, **kwargs)
 
-            attr = getattr(pkg, d)
-            if not isinstance(attr, dict):
-                raise spack.error.SpackError(
-                    "Package %s has non-dict %s attribute!" % (pkg, d))
+                # ...so if it is not a sequence make it so
+                if not isinstance(values, collections.Sequence):
+                    values = (values, )
 
-    def __call__(self, directive_function):
-        directives[directive_function.__name__] = self
+                DirectiveMetaMixin._directives_to_be_executed.extend(values)
+            return _wrapper
 
-        @functools.wraps(directive_function)
-        def wrapped(*args, **kwargs):
-            pkg = DictWrapper(caller_locals())
-            self.ensure_dicts(pkg)
+        return _decorator
 
-            pkg.name = get_calling_module_name()
-            return directive_function(pkg, *args, **kwargs)
 
-        return wrapped
+directive = DirectiveMetaMixin.directive
 
 
 @directive('versions')
-def version(pkg, ver, checksum=None, **kwargs):
+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.
+    Metadata is just stored as a dict in the package's versions
+    dictionary.  Package must turn it into a valid fetch strategy
+    later.
     """
-    # TODO: checksum vs md5 distinction is confusing -- fix this.
-    # special case checksum for backward compatibility
-    if checksum:
-        kwargs['md5'] = checksum
+    def _execute(pkg):
+        # TODO: checksum vs md5 distinction is confusing -- fix this.
+        # 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
+        # Store kwargs for the package to later with a fetch_strategy.
+        pkg.versions[Version(ver)] = kwargs
+    return _execute
 
 
 def _depends_on(pkg, spec, when=None, type=None):
@@ -199,7 +251,7 @@ def _depends_on(pkg, spec, when=None, type=None):
     if pkg.name == dep_spec.name:
         raise CircularReferenceError('depends_on', pkg.name)
 
-    pkg_deptypes = pkg._deptypes.setdefault(dep_spec.name, set())
+    pkg_deptypes = pkg.dependency_types.setdefault(dep_spec.name, set())
     for deptype in type:
         pkg_deptypes.add(deptype)
 
@@ -210,17 +262,19 @@ def _depends_on(pkg, spec, when=None, type=None):
         conditions[when_spec] = dep_spec
 
 
-@directive(('dependencies', '_deptypes'))
-def depends_on(pkg, spec, when=None, type=None):
+@directive(('dependencies', 'dependency_types'))
+def depends_on(spec, when=None, type=None):
     """Creates a dict of deps with specs defining when they apply.
     This directive is to be used inside a Package definition to declare
     that the package requires other packages to be built first.
     @see The section "Dependency specs" in the Spack Packaging Guide."""
-    _depends_on(pkg, spec, when=when, type=type)
+    def _execute(pkg):
+        _depends_on(pkg, spec, when=when, type=type)
+    return _execute
 
 
-@directive(('extendees', 'dependencies', '_deptypes'))
-def extends(pkg, spec, **kwargs):
+@directive(('extendees', 'dependencies', 'dependency_types'))
+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
@@ -234,63 +288,69 @@ def extends(pkg, spec, **kwargs):
     mechanism.
 
     """
-    if pkg.extendees:
-        raise DirectiveError("Packages can extend at most one other package.")
+    def _execute(pkg):
+        if pkg.extendees:
+            msg = 'Packages can extend at most one other package.'
+            raise DirectiveError(msg)
 
-    when = kwargs.pop('when', pkg.name)
-    _depends_on(pkg, spec, when=when)
-    pkg.extendees[spec] = (Spec(spec), kwargs)
+        when = kwargs.pop('when', pkg.name)
+        _depends_on(pkg, spec, when=when)
+        pkg.extendees[spec] = (Spec(spec), kwargs)
+    return _execute
 
 
 @directive('provided')
-def provides(pkg, *specs, **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.
     """
-    spec_string = kwargs.get('when', pkg.name)
-    provider_spec = parse_anonymous_spec(spec_string, pkg.name)
+    def _execute(pkg):
+        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
+        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
+    return _execute
 
 
 @directive('patches')
-def patch(pkg, url_or_filename, level=1, when=None, **kwargs):
+def patch(url_or_filename, level=1, when=None, **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).
+    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).
     """
-    if when is None:
-        when = pkg.name
-    when_spec = parse_anonymous_spec(when, pkg.name)
-    cur_patches = pkg.patches.setdefault(when_spec, [])
-    # if this spec is identical to some other, then append this
-    # patch to the existing list.
-    cur_patches.append(Patch.create(pkg, url_or_filename, level, **kwargs))
+    def _execute(pkg):
+        constraint = pkg.name if when is None else when
+        when_spec = parse_anonymous_spec(constraint, pkg.name)
+        cur_patches = pkg.patches.setdefault(when_spec, [])
+        # if this spec is identical to some other, then append this
+        # patch to the existing list.
+        cur_patches.append(Patch.create(pkg, url_or_filename, level, **kwargs))
+    return _execute
 
 
 @directive('variants')
-def variant(pkg, name, default=False, description=""):
+def variant(name, default=False, description=""):
     """Define a variant for the package. Packager can specify a default
     value (on or off) as well as a text description."""
-
-    default     = default
     description = str(description).strip()
 
-    if not re.match(spack.spec.identifier_re, name):
-        raise DirectiveError("Invalid variant name in %s: '%s'" %
-                             (pkg.name, name))
+    def _execute(pkg):
+        if not re.match(spack.spec.identifier_re, name):
+            msg = 'Invalid variant name in {0}: \'{1}\''
+            raise DirectiveError(msg.format(pkg.name, name))
 
-    pkg.variants[name] = Variant(default, description)
+        pkg.variants[name] = Variant(default, description)
+    return _execute
 
 
 @directive('resources')
-def resource(pkg, **kwargs):
+def resource(**kwargs):
     """Define an external resource to be fetched and staged when building the
     package. Based on the keywords present in the dictionary the appropriate
     FetchStrategy will be used for the resource. Resources are fetched and
@@ -306,29 +366,36 @@ def resource(pkg, **kwargs):
     * 'placement' : (optional) gives the possibility to fine tune how the
       resource is moved into the main package stage area.
     """
-    when = kwargs.get('when', pkg.name)
-    destination = kwargs.get('destination', "")
-    placement = kwargs.get('placement', None)
-    # Check if the path is relative
-    if os.path.isabs(destination):
-        message = "The destination keyword of a resource directive can't be"
-        " an absolute path.\n"
-        message += "\tdestination : '{dest}\n'".format(dest=destination)
-        raise RuntimeError(message)
-    # Check if the path falls within the main package stage area
-    test_path = 'stage_folder_root'
-    normalized_destination = os.path.normpath(join_path(test_path, destination)
-                                              )  # Normalized absolute path
-    if test_path not in normalized_destination:
-        message = "The destination folder of a resource must fall within the"
-        " main package stage directory.\n"
-        message += "\tdestination : '{dest}'\n".format(dest=destination)
-        raise RuntimeError(message)
-    when_spec = parse_anonymous_spec(when, pkg.name)
-    resources = pkg.resources.setdefault(when_spec, [])
-    name = kwargs.get('name')
-    fetcher = from_kwargs(**kwargs)
-    resources.append(Resource(name, fetcher, destination, placement))
+    def _execute(pkg):
+        when = kwargs.get('when', pkg.name)
+        destination = kwargs.get('destination', "")
+        placement = kwargs.get('placement', None)
+
+        # Check if the path is relative
+        if os.path.isabs(destination):
+            message = 'The destination keyword of a resource directive '
+            'can\'t be an absolute path.\n'
+            message += "\tdestination : '{dest}\n'".format(dest=destination)
+            raise RuntimeError(message)
+
+        # Check if the path falls within the main package stage area
+        test_path = 'stage_folder_root'
+        normalized_destination = os.path.normpath(
+            join_path(test_path, destination)
+        )  # Normalized absolute path
+
+        if test_path not in normalized_destination:
+            message = "The destination folder of a resource must fall "
+            "within the main package stage directory.\n"
+            message += "\tdestination : '{dest}'\n".format(dest=destination)
+            raise RuntimeError(message)
+
+        when_spec = parse_anonymous_spec(when, pkg.name)
+        resources = pkg.resources.setdefault(when_spec, [])
+        name = kwargs.get('name')
+        fetcher = from_kwargs(**kwargs)
+        resources.append(Resource(name, fetcher, destination, placement))
+    return _execute
 
 
 class DirectiveError(spack.error.SpackError):
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 0c924cfcbc..f9bc1fafbc 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -134,7 +134,7 @@ class InstallPhase(object):
             return other
 
 
-class PackageMeta(type):
+class PackageMeta(spack.directives.DirectiveMetaMixin):
     """Conveniently transforms attributes to permit extensible phases
 
     Iterates over the attribute 'phases' and creates / updates private
@@ -232,10 +232,6 @@ class PackageMeta(type):
         _append_checks('sanity_checks')
         return super(PackageMeta, meta).__new__(meta, name, bases, attr_dict)
 
-    def __init__(cls, name, bases, dict):
-        type.__init__(cls, name, bases, dict)
-        spack.directives.ensure_dicts(cls)
-
 
 class PackageBase(object):
     """This is the superclass for all spack packages.
@@ -781,7 +777,7 @@ class PackageBase(object):
     def dependencies_of_type(self, *deptypes):
         """Get subset of the dependencies with certain types."""
         return dict((name, conds) for name, conds in self.dependencies.items()
-                    if any(d in self._deptypes[name] for d in deptypes))
+                    if any(d in self.dependency_types[name] for d in deptypes))
 
     @property
     def extendee_spec(self):
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 8230142d43..5eed6c71c6 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1762,7 +1762,7 @@ class Spec(object):
             for dep_name in pkg.dependencies:
                 # Do we depend on dep_name?  If so pkg_dep is not None.
                 pkg_dep = self._evaluate_dependency_conditions(dep_name)
-                deptypes = pkg._deptypes[dep_name]
+                deptypes = pkg.dependency_types[dep_name]
                 # If pkg_dep is a dependency, merge it.
                 if pkg_dep:
                     changed |= self._merge_dependency(
diff --git a/lib/spack/spack/test/mock_packages_test.py b/lib/spack/spack/test/mock_packages_test.py
index eab96165e0..69c52c2f53 100644
--- a/lib/spack/spack/test/mock_packages_test.py
+++ b/lib/spack/spack/test/mock_packages_test.py
@@ -255,7 +255,7 @@ class MockPackagesTest(unittest.TestCase):
         # Change dep spec
         # XXX(deptype): handle deptypes.
         pkg.dependencies[spec.name] = {Spec(pkg_name): spec}
-        pkg._deptypes[spec.name] = set(deptypes)
+        pkg.dependency_types[spec.name] = set(deptypes)
 
     def cleanmock(self):
         """Restore the real packages path after any test."""
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index fdd079a8f7..1217568c9c 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -27,6 +27,7 @@ from llnl.util.filesystem import join_path
 from spack.repository import Repo
 from spack.test.mock_packages_test import *
 from spack.util.naming import mod_to_class
+from spack.spec import *
 
 
 class PackagesTest(MockPackagesTest):
@@ -89,3 +90,28 @@ class PackagesTest(MockPackagesTest):
         import spack.pkg.builtin.mock                   # noqa
         import spack.pkg.builtin.mock as m              # noqa
         from spack.pkg.builtin import mock              # noqa
+
+    def test_inheritance_of_diretives(self):
+        p = spack.repo.get('simple_inheritance')
+
+        # Check dictionaries that should have been filled by directives
+        self.assertEqual(len(p.dependencies), 3)
+        self.assertTrue('cmake' in p.dependencies)
+        self.assertTrue('openblas' in p.dependencies)
+        self.assertTrue('mpi' in p.dependencies)
+        self.assertEqual(len(p.provided), 2)
+
+        # Check that Spec instantiation behaves as we expect
+        s = Spec('simple_inheritance')
+        s.concretize()
+        self.assertTrue('^cmake' in s)
+        self.assertTrue('^openblas' in s)
+        self.assertTrue('+openblas' in s)
+        self.assertTrue('mpi' in s)
+
+        s = Spec('simple_inheritance~openblas')
+        s.concretize()
+        self.assertTrue('^cmake' in s)
+        self.assertTrue('^openblas' not in s)
+        self.assertTrue('~openblas' in s)
+        self.assertTrue('mpi' in s)