Refactor environment setup.

- Gave setup_environment and setup_dependent_environment more similar
  signatures. They now allows editing the Spack env and the runtime
  env for *this* package and dependents, respectively.

- modify_module renamed to setup_dependent_python_module for symmetry
  with setup_dependent_environment and to avoid confusion with
  environment modules.

- removed need for patching Package objects at runtime.

- adjust packages to reflect these changes.

9 files changed, 151 insertions, 97 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 59b234624c..5688d47e2d 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -84,7 +84,7 @@ class MakeExecutable(Executable):
         return super(MakeExecutable, self).__call__(*args, **kwargs)
 
 
-def set_compiler_environment_variables(pkg):
+def set_compiler_environment_variables(pkg, env):
     assert pkg.spec.concrete
     # Set compiler variables used by CMake and autotools
     assert all(key in pkg.compiler.link_paths for key in ('cc', 'cxx', 'f77', 'fc'))
@@ -92,7 +92,6 @@ def set_compiler_environment_variables(pkg):
     # Populate an object with the list of environment modifications
     # and return it
     # TODO : add additional kwargs for better diagnostics, like requestor, ttyout, ttyerr, etc.
-    env = EnvironmentModifications()
     link_dir = spack.build_env_path
     env.set_env('CC', join_path(link_dir, pkg.compiler.link_paths['cc']))
     env.set_env('CXX', join_path(link_dir, pkg.compiler.link_paths['cxx']))
@@ -113,7 +112,7 @@ def set_compiler_environment_variables(pkg):
     return env
 
 
-def set_build_environment_variables(pkg):
+def set_build_environment_variables(pkg, env):
     """
     This ensures a clean install environment when we build packages
     """
@@ -134,7 +133,6 @@ def set_build_environment_variables(pkg):
         if os.path.isdir(ci):
             env_paths.append(ci)
 
-    env = EnvironmentModifications()
     for item in reversed(env_paths):
         env.prepend_path('PATH', item)
     env.set_env(SPACK_ENV_PATH, concatenate_paths(env_paths))
@@ -180,7 +178,7 @@ def set_build_environment_variables(pkg):
     return env
 
 
-def set_module_variables_for_package(pkg, m):
+def set_module_variables_for_package(pkg, module):
     """Populate the module scope of install() with some useful functions.
        This makes things easier for package writers.
     """
@@ -190,6 +188,8 @@ def set_module_variables_for_package(pkg, m):
         jobs = 1
     elif pkg.make_jobs:
         jobs = pkg.make_jobs
+
+    m = module
     m.make_jobs = jobs
 
     # TODO: make these build deps that can be installed if not found.
@@ -271,9 +271,12 @@ def parent_class_modules(cls):
 
 def setup_package(pkg):
     """Execute all environment setup routines."""
-    env = EnvironmentModifications()
-    env.extend(set_compiler_environment_variables(pkg))
-    env.extend(set_build_environment_variables(pkg))
+    spack_env = EnvironmentModifications()
+    run_env   = EnvironmentModifications()
+
+    set_compiler_environment_variables(pkg, spack_env)
+    set_build_environment_variables(pkg, spack_env)
+
     # If a user makes their own package repo, e.g.
     # spack.repos.mystuff.libelf.Libelf, and they inherit from
     # an existing class like spack.repos.original.libelf.Libelf,
@@ -285,12 +288,20 @@ def setup_package(pkg):
 
     # Allow dependencies to modify the module
     for dependency_spec in pkg.spec.traverse(root=False):
-        dependency_spec.package.modify_module(pkg.module, dependency_spec, pkg.spec)
+        dpkg = dependency_spec.package
+        dpkg.setup_dependent_python_module(pkg.module, pkg.spec)
+
     # Allow dependencies to set up environment as well
     for dependency_spec in pkg.spec.traverse(root=False):
-        dependency_spec.package.setup_dependent_environment(env, pkg.spec)
-    validate(env, tty.warn)
-    env.apply_modifications()
+        dpkg = dependency_spec.package
+        dpkg.setup_dependent_environment(spack_env, run_env, pkg.spec)
+
+    # Allow the package to apply some settings.
+    pkg.setup_environment(spack_env, run_env)
+
+    # Make sure nothing's strange about the Spack environment.
+    validate(spack_env, tty.warn)
+    spack_env.apply_modifications()
 
 
 def fork(pkg, function):
diff --git a/lib/spack/spack/modules.py b/lib/spack/spack/modules.py
index 4e98d50001..05c93cd3e6 100644
--- a/lib/spack/spack/modules.py
+++ b/lib/spack/spack/modules.py
@@ -164,7 +164,8 @@ class EnvModule(object):
                 self.pkg.module, extendee_spec, self.spec)
 
         # Package-specific environment modifications
-        self.spec.package.setup_environment(env)
+        spack_env = EnvironmentModifications()
+        self.spec.package.setup_environment(spack_env, env)
 
         # TODO : implement site-specific modifications and filters
         if not env:
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index acad5a28f6..9d8ac87bd7 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1002,56 +1002,120 @@ class Package(object):
         return __import__(self.__class__.__module__,
                           fromlist=[self.__class__.__name__])
 
-    def setup_environment(self, env):
-        """
-        Appends in `env` the list of environment modifications needed to use this package outside of spack.
+    def setup_environment(self, spack_env, run_env):
+        """Set up the compile and runtime environemnts for a package.
 
-        Default implementation does nothing, but this can be overridden if the package needs a particular environment.
+        `spack_env` and `run_env` are `EnvironmentModifications`
+        objects.  Package authors can call methods on them to alter
+        the environment within Spack and at runtime.
 
-        Example :
+        Both `spack_env` and `run_env` are applied within the build
+        process, before this package's `install()` method is called.
+
+        Modifications in `run_env` will *also* be added to the
+        generated environment modules for this package.
+
+        Default implementation does nothing, but this can be
+        overridden if the package needs a particular environment.
 
-        1. A lot of Qt extensions need `QTDIR` set.  This can be used to do that.
+        Examples:
+
+            1. Qt extensions need `QTDIR` set.
 
         Args:
-            env: list of environment modifications to be updated
+            spack_env (EnvironmentModifications): list of
+                modifications to be applied when this package is built
+                within Spack.
+
+            run_env (EnvironmentModifications): list of environment
+                changes to be applied when this package is run outside
+                of Spack.
+
         """
         pass
 
-    def setup_dependent_environment(self, env, dependent_spec):
-        """
-        Called before the install() method of dependents.
 
-        Appends in `env` the list of environment modifications needed by dependents (or extensions) during the
-        installation of a package. The default implementation delegates to `setup_environment`, but can be overridden
-        if the modifications to the environment happen to be different from the one needed to use the package outside
-        of spack.
+    def setup_dependent_environment(self, spack_env, run_env, dependent_spec):
+        """Set up the environment of packages that depend on this one.
+
+        This is similar to `setup_environment`, but it is used to
+        modify the compile and runtime environments of packages that
+        *depend* on this one. This gives packages like Python and
+        others that follow the extension model a way to implement
+        common environment or compile-time settings for dependencies.
 
-        This is useful if there are some common steps to installing all extensions for a certain package.
+        By default, this delegates to self.setup_environment()
 
         Example :
 
-        1. Installing python modules generally requires `PYTHONPATH` to point to the lib/pythonX.Y/site-packages
-        directory in the module's install prefix.  This could set that variable.
+            1. Installing python modules generally requires
+              `PYTHONPATH` to point to the lib/pythonX.Y/site-packages
+              directory in the module's install prefix.  This could
+              set that variable.
 
         Args:
-            env: list of environment modifications to be updated
-            dependent_spec: dependent (or extension) of this spec
-        """
-        self.setup_environment(env)
 
-    def modify_module(self, module, spec, dependent_spec):
+            spack_env (EnvironmentModifications): list of
+                modifications to be applied when the dependent package
+                is bulit within Spack.
+
+            run_env (EnvironmentModifications): list of environment
+                changes to be applied when the dependent package is
+                run outside of Spack.
+
+            dependent_spec (Spec): The spec of the dependent package
+                about to be built. This allows the extendee (self) to
+                query the dependent's state. Note that *this*
+                package's spec is available as `self.spec`.
+
+        This is useful if there are some common steps to installing
+        all extensions for a certain package.
+
         """
+        self.setup_environment(spack_env, run_env)
+
+
+    def setup_dependent_python_module(self, module, dependent_spec):
+        """Set up Python module-scope variables for dependent packages.
+
         Called before the install() method of dependents.
 
-        Default implementation does nothing, but this can be overridden by an extendable package to set up the module of
-        its extensions. This is useful if there are some common steps to installing all extensions for a
-        certain package.
+        Default implementation does nothing, but this can be
+        overridden by an extendable package to set up the module of
+        its extensions. This is useful if there are some common steps
+        to installing all extensions for a certain package.
 
         Example :
 
-        1. Extensions often need to invoke the 'python' interpreter from the Python installation being extended.
-        This routine can put a 'python' Executable object in the module scope for the extension package to simplify
-        extension installs.
+            1. Extensions often need to invoke the `python`
+               interpreter from the Python installation being
+               extended.  This routine can put a 'python' Executable
+               object in the module scope for the extension package to
+               simplify extension installs.
+
+            2. MPI compilers could set some variables in the
+               dependent's scope that point to `mpicc`, `mpicxx`,
+               etc., allowing them to be called by common names
+               regardless of which MPI is used.
+
+            3. BLAS/LAPACK implementations can set some variables
+               indicating the path to their libraries, since these
+               paths differ by BLAS/LAPACK implementation.
+
+        Args:
+
+            module (module): The Python `module` object of the
+                dependent package. Packages can use this to set
+                module-scope variables for the dependent to use.
+
+            dependent_spec (Spec): The spec of the dependent package
+                about to be built. This allows the extendee (self) to
+                query the dependent's state.  Note that *this*
+                package's spec is available as `self.spec`.
+
+        This is useful if there are some common steps to installing
+        all extensions for a certain package.
+
         """
         pass
 
diff --git a/var/spack/repos/builtin/packages/mpich/package.py b/var/spack/repos/builtin/packages/mpich/package.py
index 5af9b585ea..90b5d42eab 100644
--- a/var/spack/repos/builtin/packages/mpich/package.py
+++ b/var/spack/repos/builtin/packages/mpich/package.py
@@ -47,13 +47,6 @@ class Mpich(Package):
     provides('mpi@:3.0', when='@3:')
     provides('mpi@:1.3', when='@1:')
 
-    def setup_environment(self, env):
-        env.set_env('MPICH_CC', self.compiler.cc)
-        env.set_env('MPICH_CXX', self.compiler.cxx)
-        env.set_env('MPICH_F77', self.compiler.f77)
-        env.set_env('MPICH_F90', self.compiler.fc)
-        env.set_env('MPICH_FC', self.compiler.fc)
-
     def setup_dependent_environment(self, env, dependent_spec):
         env.set_env('MPICH_CC', spack_cc)
         env.set_env('MPICH_CXX', spack_cxx)
@@ -61,7 +54,7 @@ class Mpich(Package):
         env.set_env('MPICH_F90', spack_f90)
         env.set_env('MPICH_FC', spack_fc)
 
-    def modify_module(self, module, spec, dep_spec):
+    def setup_dependent_python_module(self, module, spec, dep_spec):
         """For dependencies, make mpicc's use spack wrapper."""
         # FIXME : is this necessary ? Shouldn't this be part of a contract with MPI providers?
         module.mpicc = join_path(self.prefix.bin, 'mpicc')
diff --git a/var/spack/repos/builtin/packages/netlib-scalapack/package.py b/var/spack/repos/builtin/packages/netlib-scalapack/package.py
index 36f506f7cd..62abfcc48e 100644
--- a/var/spack/repos/builtin/packages/netlib-scalapack/package.py
+++ b/var/spack/repos/builtin/packages/netlib-scalapack/package.py
@@ -40,7 +40,7 @@ class NetlibScalapack(Package):
             make()
             make("install")
 
-    def modify_module(self, module, spec, dependent_spec):
+    def setup_dependent_python_module(self, module, spec, dependent_spec):
         lib_dsuffix = '.dylib' if sys.platform == 'darwin' else '.so'
         lib_suffix = lib_dsuffix if '+shared' in spec['scalapack'] else '.a'
 
diff --git a/var/spack/repos/builtin/packages/openmpi/package.py b/var/spack/repos/builtin/packages/openmpi/package.py
index 7783ca8766..c91a13e376 100644
--- a/var/spack/repos/builtin/packages/openmpi/package.py
+++ b/var/spack/repos/builtin/packages/openmpi/package.py
@@ -41,17 +41,13 @@ class Openmpi(Package):
     def url_for_version(self, version):
         return "http://www.open-mpi.org/software/ompi/v%s/downloads/openmpi-%s.tar.bz2" % (version.up_to(2), version)
 
-    def setup_environment(self, env):
-        env.set_env('OMPI_CC', self.compiler.cc)
-        env.set_env('OMPI_CXX', self.compiler.cxx)
-        env.set_env('OMPI_FC', self.compiler.fc)
-        env.set_env('OMPI_F77', self.compiler.f77)
-
-    def setup_dependent_environment(self, env, dependent_spec):
-        env.set_env('OMPI_CC', spack_cc)
-        env.set_env('OMPI_CXX', spack_cxx)
-        env.set_env('OMPI_FC', spack_fc)
-        env.set_env('OMPI_F77', spack_f77)
+
+    def setup_dependent_environment(self, spack_env, run_env, dependent_spec):
+        spack_env.set_env('OMPI_CC', spack_cc)
+        spack_env.set_env('OMPI_CXX', spack_cxx)
+        spack_env.set_env('OMPI_FC', spack_fc)
+        spack_env.set_env('OMPI_F77', spack_f77)
+
 
     def install(self, spec, prefix):
         config_args = ["--prefix=%s" % prefix,
diff --git a/var/spack/repos/builtin/packages/python/package.py b/var/spack/repos/builtin/packages/python/package.py
index c445d26369..593a27708c 100644
--- a/var/spack/repos/builtin/packages/python/package.py
+++ b/var/spack/repos/builtin/packages/python/package.py
@@ -92,13 +92,21 @@ class Python(Package):
     def site_packages_dir(self):
         return os.path.join(self.python_lib_dir, 'site-packages')
 
-    def setup_dependent_environment(self, env, extension_spec):
-        # Set PYTHONPATH to include site-packages dir for the extension and any other python extensions it depends on.
+
+    def setup_dependent_environment(self, spack_env, run_env, extension_spec):
+        # TODO: do this only for actual extensions.
+
+        # Set PYTHONPATH to include site-packages dir for the
+        # extension and any other python extensions it depends on.
         python_paths = []
         for d in extension_spec.traverse():
             if d.package.extends(self.spec):
                 python_paths.append(os.path.join(d.prefix, self.site_packages_dir))
-        env.set_env('PYTHONPATH', ':'.join(python_paths))
+
+        pythonpath = ':'.join(python_paths)
+        spack_env.set_env('PYTHONPATH', pythonpath)
+        run_env.set_env('PYTHONPATH', pythonpath)
+
 
     def modify_module(self, module, spec, ext_spec):
         """
@@ -114,31 +122,6 @@ class Python(Package):
         else:
             module.python = Executable(join_path(spec.prefix.bin, 'python'))
 
-        # The code below patches the any python extension to have good defaults for `setup_dependent_environment` and
-        # `setup_environment` only if the extension didn't override any of these functions explicitly.
-        def _setup_env(self, env):
-            site_packages = glob.glob(join_path(self.spec.prefix.lib, "python*/site-packages"))
-            if site_packages:
-                env.prepend_path('PYTHONPATH', site_packages[0])
-
-        def _setup_denv(self, env, extension_spec):
-            pass
-
-        pkg_cls = type(ext_spec.package)  # Retrieve the type we may want to patch
-        if 'python' in pkg_cls.extendees:
-            # List of overrides we are interested in
-            interesting_overrides = ['setup_environment', 'setup_dependent_environment']
-            overrides_found = [
-                (name, defining_cls) for name, _, defining_cls, _, in inspect.classify_class_attrs(pkg_cls)
-                if
-                name in interesting_overrides and  # The attribute has the right name
-                issubclass(defining_cls, Package) and defining_cls is not Package  # and is an actual override
-            ]
-            if not overrides_found:
-                # If no override were found go on patching
-                pkg_cls.setup_environment = functools.wraps(Package.setup_environment)(_setup_env)
-                pkg_cls.setup_dependent_environment = functools.wraps(Package.setup_dependent_environment)(_setup_denv)
-
         # Add variables for lib/pythonX.Y and lib/pythonX.Y/site-packages dirs.
         module.python_lib_dir     = os.path.join(ext_spec.prefix, self.python_lib_dir)
         module.python_include_dir = os.path.join(ext_spec.prefix, self.python_include_dir)
diff --git a/var/spack/repos/builtin/packages/qt/package.py b/var/spack/repos/builtin/packages/qt/package.py
index 35b9d68462..039aeb3c31 100644
--- a/var/spack/repos/builtin/packages/qt/package.py
+++ b/var/spack/repos/builtin/packages/qt/package.py
@@ -55,8 +55,14 @@ class Qt(Package):
     depends_on("mesa", when='@4:+mesa')
     depends_on("libxcb")
 
-    def setup_environment(self, env):
-        env.set_env['QTDIR'] = self.prefix
+
+    def setup_environment(self, spack_env, env):
+        env.set_env('QTDIR', self.prefix)
+
+
+    def setup_dependent_environment(self, spack_env, run_env, dspec):
+        spack_env.set_env('QTDIR', self.prefix)
+
 
     def patch(self):
         if self.spec.satisfies('@4'):
diff --git a/var/spack/repos/builtin/packages/ruby/package.py b/var/spack/repos/builtin/packages/ruby/package.py
index 2d1da8c9af..39f65f51d2 100644
--- a/var/spack/repos/builtin/packages/ruby/package.py
+++ b/var/spack/repos/builtin/packages/ruby/package.py
@@ -2,7 +2,7 @@ from spack import *
 
 
 class Ruby(Package):
-    """A dynamic, open source programming language with a focus on 
+    """A dynamic, open source programming language with a focus on
     simplicity and productivity."""
 
     homepage = "https://www.ruby-lang.org/"
@@ -17,15 +17,17 @@ class Ruby(Package):
         make()
         make("install")
 
-    def setup_dependent_environment(self, env, extension_spec):
+    def setup_dependent_environment(self, spack_env, run_env, extension_spec):
+        # TODO: do this only for actual extensions.
         # Set GEM_PATH to include dependent gem directories
         ruby_paths = []
         for d in extension_spec.traverse():
             if d.package.extends(self.spec):
                 ruby_paths.append(d.prefix)
-        env.set_env('GEM_PATH', concatenate_paths(ruby_paths))
+
+        spack_env.set_env('GEM_PATH', concatenate_paths(ruby_paths))
         # The actual installation path for this gem
-        env.set_env('GEM_HOME', extension_spec.prefix)
+        spack_env.set_env('GEM_HOME', extension_spec.prefix)
 
     def modify_module(self, module, spec, ext_spec):
         """Called before ruby modules' install() methods.  Sets GEM_HOME
@@ -38,5 +40,3 @@ class Ruby(Package):
         # Ruby extension builds have global ruby and gem functions
         module.ruby = Executable(join_path(spec.prefix.bin, 'ruby'))
         module.gem = Executable(join_path(spec.prefix.bin, 'gem'))
-
-