Merge pull request #552 from epfl-scitas/features/env_objects_flying_around

enhancement proposal : customization of module files

lib/spack/spack/build_environment.py

@@ -3,7 +3,7 @@
 build environment.  All of this is set up by package.py just before
 install() is called.
 
-There are two parts to the bulid environment:
+There are two parts to the build environment:
 
 1. Python build environment (i.e. install() method)
 
@@ -13,7 +13,7 @@
    the package's module scope.  Ths allows package writers to call
    them all directly in Package.install() without writing 'self.'
    everywhere.  No, this isn't Pythonic.  Yes, it makes the code more
-   readable and more like the shell script from whcih someone is
+   readable and more like the shell script from which someone is
    likely porting.
 
 2. Build execution environment
@@ -27,17 +27,18 @@
 Skimming this module is a nice way to get acquainted with the types of
 calls you can make from within the install() function.
 """
-import os
-import sys
-import shutil
 import multiprocessing
+import os
 import platform
-from llnl.util.filesystem import *
+import shutil
+import sys
 
 import spack
-import spack.compilers as compilers
+import llnl.util.tty as tty
-from spack.util.executable import Executable, which
+from llnl.util.filesystem import *
+from spack.environment import EnvironmentModifications, concatenate_paths, validate
 from spack.util.environment import *
+from spack.util.executable import Executable, which
 
 #
 # This can be set by the user to globally disable parallel builds.
@@ -84,84 +85,88 @@ def __call__(self, *args, **kwargs):
 
 
 def set_compiler_environment_variables(pkg):
-    assert(pkg.spec.concrete)
+    assert pkg.spec.concrete
-    compiler = pkg.compiler
-
     # Set compiler variables used by CMake and autotools
-    assert all(key in pkg.compiler.link_paths
+    assert all(key in pkg.compiler.link_paths for key in ('cc', 'cxx', 'f77', 'fc'))
-               for key in ('cc', 'cxx', 'f77', 'fc'))
 
+    # 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
-    os.environ['CC']  = join_path(link_dir, pkg.compiler.link_paths['cc'])
+    env.set_env('CC', join_path(link_dir, pkg.compiler.link_paths['cc']))
-    os.environ['CXX'] = join_path(link_dir, pkg.compiler.link_paths['cxx'])
+    env.set_env('CXX', join_path(link_dir, pkg.compiler.link_paths['cxx']))
-    os.environ['F77'] = join_path(link_dir, pkg.compiler.link_paths['f77'])
+    env.set_env('F77', join_path(link_dir, pkg.compiler.link_paths['f77']))
-    os.environ['FC']  = join_path(link_dir, pkg.compiler.link_paths['fc'])
+    env.set_env('FC', join_path(link_dir, pkg.compiler.link_paths['fc']))
-
     # Set SPACK compiler variables so that our wrapper knows what to call
+    compiler = pkg.compiler
     if compiler.cc:
-        os.environ['SPACK_CC']  = compiler.cc
+        env.set_env('SPACK_CC', compiler.cc)
     if compiler.cxx:
-        os.environ['SPACK_CXX'] = compiler.cxx
+        env.set_env('SPACK_CXX', compiler.cxx)
     if compiler.f77:
-        os.environ['SPACK_F77'] = compiler.f77
+        env.set_env('SPACK_F77', compiler.f77)
     if compiler.fc:
-        os.environ['SPACK_FC']  = compiler.fc
+        env.set_env('SPACK_FC', compiler.fc)
 
-    os.environ['SPACK_COMPILER_SPEC']  = str(pkg.spec.compiler)
+    env.set_env('SPACK_COMPILER_SPEC', str(pkg.spec.compiler))
+    return env
 
 
 def set_build_environment_variables(pkg):
-    """This ensures a clean install environment when we build packages.
+    """
+    This ensures a clean install environment when we build packages
     """
     # Add spack build environment path with compiler wrappers first in
     # the path. We add both spack.env_path, which includes default
     # wrappers (cc, c++, f77, f90), AND a subdirectory containing
     # compiler-specific symlinks.  The latter ensures that builds that
     # are sensitive to the *name* of the compiler see the right name
-    # when we're building wtih the wrappers.
+    # when we're building with the wrappers.
     #
     # Conflicts on case-insensitive systems (like "CC" and "cc") are
     # handled by putting one in the <build_env_path>/case-insensitive
     # directory.  Add that to the path too.
     env_paths = []
-    def add_env_path(path):
+    for item in [spack.build_env_path, join_path(spack.build_env_path, pkg.compiler.name)]:
-        env_paths.append(path)
+        env_paths.append(item)
-        ci = join_path(path, 'case-insensitive')
+        ci = join_path(item, 'case-insensitive')
-        if os.path.isdir(ci): env_paths.append(ci)
+        if os.path.isdir(ci):
-    add_env_path(spack.build_env_path)
+            env_paths.append(ci)
-    add_env_path(join_path(spack.build_env_path, pkg.compiler.name))
 
-    path_put_first("PATH", env_paths)
+    env = EnvironmentModifications()
-    path_set(SPACK_ENV_PATH, env_paths)
+    for item in reversed(env_paths):
+        env.prepend_path('PATH', item)
+    env.set_env(SPACK_ENV_PATH, concatenate_paths(env_paths))
 
-    # Prefixes of all of the package's dependencies go in
+    # Prefixes of all of the package's dependencies go in SPACK_DEPENDENCIES
-    # SPACK_DEPENDENCIES
     dep_prefixes = [d.prefix for d in pkg.spec.traverse(root=False)]
-    path_set(SPACK_DEPENDENCIES, dep_prefixes)
+    env.set_env(SPACK_DEPENDENCIES, concatenate_paths(dep_prefixes))
+    env.set_env('CMAKE_PREFIX_PATH', concatenate_paths(dep_prefixes))  # Add dependencies to CMAKE_PREFIX_PATH
 
     # Install prefix
-    os.environ[SPACK_PREFIX] = pkg.prefix
+    env.set_env(SPACK_PREFIX, pkg.prefix)
 
     # Install root prefix
-    os.environ[SPACK_INSTALL] = spack.install_path
+    env.set_env(SPACK_INSTALL, spack.install_path)
 
     # Remove these vars from the environment during build because they
     # can affect how some packages find libraries.  We want to make
     # sure that builds never pull in unintended external dependencies.
-    pop_keys(os.environ, "LD_LIBRARY_PATH", "LD_RUN_PATH", "DYLD_LIBRARY_PATH")
+    env.unset_env('LD_LIBRARY_PATH')
+    env.unset_env('LD_RUN_PATH')
+    env.unset_env('DYLD_LIBRARY_PATH')
 
     # Add bin directories from dependencies to the PATH for the build.
-    bin_dirs = ['%s/bin' % prefix for prefix in dep_prefixes]
+    bin_dirs = reversed(filter(os.path.isdir, ['%s/bin' % prefix for prefix in dep_prefixes]))
-    path_put_first('PATH', [bin for bin in bin_dirs if os.path.isdir(bin)])
+    for item in bin_dirs:
+        env.prepend_path('PATH', item)
 
     # Working directory for the spack command itself, for debug logs.
     if spack.debug:
-        os.environ[SPACK_DEBUG] = "TRUE"
+        env.set_env(SPACK_DEBUG, 'TRUE')
-    os.environ[SPACK_SHORT_SPEC] = pkg.spec.short_spec
+    env.set_env(SPACK_SHORT_SPEC, pkg.spec.short_spec)
-    os.environ[SPACK_DEBUG_LOG_DIR] = spack.spack_working_dir
+    env.set_env(SPACK_DEBUG_LOG_DIR, spack.spack_working_dir)
-
-    # Add dependencies to CMAKE_PREFIX_PATH
-    path_set("CMAKE_PREFIX_PATH", dep_prefixes)
 
     # Add any pkgconfig directories to PKG_CONFIG_PATH
     pkg_config_dirs = []
@@ -170,7 +175,9 @@ def add_env_path(path):
             pcdir = join_path(p, libdir, 'pkgconfig')
             if os.path.isdir(pcdir):
                 pkg_config_dirs.append(pcdir)
-    path_set("PKG_CONFIG_PATH", pkg_config_dirs)
+    env.set_env('PKG_CONFIG_PATH', concatenate_paths(pkg_config_dirs))
+
+    return env
 
 
 def set_module_variables_for_package(pkg, m):
@@ -264,9 +271,9 @@ def parent_class_modules(cls):
 
 def setup_package(pkg):
     """Execute all environment setup routines."""
-    set_compiler_environment_variables(pkg)
+    env = EnvironmentModifications()
-    set_build_environment_variables(pkg)
+    env.extend(set_compiler_environment_variables(pkg))
-
+    env.extend(set_build_environment_variables(pkg))
     # 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,
@@ -276,10 +283,14 @@ def setup_package(pkg):
     for mod in modules:
         set_module_variables_for_package(pkg, mod)
 
-    # Allow dependencies to set up environment as well.
+    # Allow dependencies to modify the module
-    for dep_spec in pkg.spec.traverse(root=False):
+    for dependency_spec in pkg.spec.traverse(root=False):
-        dep_spec.package.setup_dependent_environment(
+        dependency_spec.package.modify_module(pkg.module, dependency_spec, pkg.spec)
-            pkg.module, dep_spec, 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()
 
 
 def fork(pkg, function):
@@ -296,23 +307,23 @@ def child_fun():
            # do stuff
        build_env.fork(pkg, child_fun)
 
-    Forked processes are run with the build environemnt set up by
+    Forked processes are run with the build environment set up by
     spack.build_environment.  This allows package authors to have
-    full control over the environment, etc. without offecting
+    full control over the environment, etc. without affecting
     other builds that might be executed in the same spack call.
 
-    If something goes wrong, the child process is expected toprint
+    If something goes wrong, the child process is expected to print
     the error and the parent process will exit with error as
     well. If things go well, the child exits and the parent
     carries on.
     """
     try:
         pid = os.fork()
-    except OSError, e:
+    except OSError as e:
         raise InstallError("Unable to fork build process: %s" % e)
 
     if pid == 0:
-        # Give the child process the package's build environemnt.
+        # Give the child process the package's build environment.
         setup_package(pkg)
 
         try:
@@ -323,7 +334,7 @@ def child_fun():
             # which interferes with unit tests.
             os._exit(0)
 
-        except spack.error.SpackError, e:
+        except spack.error.SpackError as e:
             e.die()
 
         except:
@@ -338,8 +349,7 @@ def child_fun():
         # message.  Just make the parent exit with an error code.
         pid, returncode = os.waitpid(pid, 0)
         if returncode != 0:
-            raise InstallError("Installation process had nonzero exit code."
+            raise InstallError("Installation process had nonzero exit code.".format(str(returncode)))
-                .format(str(returncode)))
 
 
 class InstallError(spack.error.SpackError):

lib/spack/spack/cmd/module.py

@@ -80,7 +80,7 @@ def module_find(mtype, spec_array):
     if not os.path.isfile(mod.file_name):
         tty.die("No %s module is installed for %s" % (mtype, spec))
 
-    print mod.use_name
+    print(mod.use_name)
 
 
 def module_refresh():

lib/spack/spack/cmd/uninstall.py

@@ -79,7 +79,7 @@ def uninstall(parser, args):
                 try:
                     # should work if package is known to spack
                     pkgs.append(s.package)
-                except spack.repository.UnknownPackageError, e:
+                except spack.repository.UnknownPackageError as e:
                     # The package.py file has gone away -- but still
                     # want to uninstall.
                     spack.Package(s).do_uninstall(force=True)
@@ -94,11 +94,11 @@ def num_installed_deps(pkg):
         for pkg in pkgs:
             try:
                 pkg.do_uninstall(force=args.force)
-            except PackageStillNeededError, e:
+            except PackageStillNeededError as e:
                 tty.error("Will not uninstall %s" % e.spec.format("$_$@$%@$#", color=True))
-                print
+                print('')
-                print "The following packages depend on it:"
+                print("The following packages depend on it:")
                 display_specs(e.dependents, long=True)
-                print
+                print('')
-                print "You can use spack uninstall -f to force this action."
+                print("You can use spack uninstall -f to force this action.")
                 sys.exit(1)

lib/spack/spack/directory_layout.py

@@ -150,7 +150,7 @@ def remove_install_directory(self, spec):
         if os.path.exists(path):
             try:
                 shutil.rmtree(path)
-            except exceptions.OSError, e:
+            except exceptions.OSError as e:
                 raise RemoveFailedError(spec, path, e)
 
         path = os.path.dirname(path)

lib/spack/spack/environment.py

@@ -0,0 +1,234 @@
+import os
+import os.path
+import collections
+import inspect
+
+
+class NameModifier(object):
+    def __init__(self, name, **kwargs):
+        self.name = name
+        self.args = {'name': name}
+        self.args.update(kwargs)
+
+
+class NameValueModifier(object):
+    def __init__(self, name, value, **kwargs):
+        self.name = name
+        self.value = value
+        self.args = {'name': name, 'value': value}
+        self.args.update(kwargs)
+
+
+class SetEnv(NameValueModifier):
+    def execute(self):
+        os.environ[self.name] = str(self.value)
+
+
+class UnsetEnv(NameModifier):
+    def execute(self):
+        os.environ.pop(self.name, None)  # Avoid throwing if the variable was not set
+
+
+class AppendPath(NameValueModifier):
+    def execute(self):
+        environment_value = os.environ.get(self.name, '')
+        directories = environment_value.split(':') if environment_value else []
+        directories.append(os.path.normpath(self.value))
+        os.environ[self.name] = ':'.join(directories)
+
+
+class PrependPath(NameValueModifier):
+    def execute(self):
+        environment_value = os.environ.get(self.name, '')
+        directories = environment_value.split(':') if environment_value else []
+        directories = [os.path.normpath(self.value)] + directories
+        os.environ[self.name] = ':'.join(directories)
+
+
+class RemovePath(NameValueModifier):
+    def execute(self):
+        environment_value = os.environ.get(self.name, '')
+        directories = environment_value.split(':') if environment_value else []
+        directories = [os.path.normpath(x) for x in directories if x != os.path.normpath(self.value)]
+        os.environ[self.name] = ':'.join(directories)
+
+
+class EnvironmentModifications(object):
+    """
+    Keeps track of requests to modify the current environment.
+
+    Each call to a method to modify the environment stores the extra information on the caller in the request:
+    - 'filename' : filename of the module where the caller is defined
+    - 'lineno': line number where the request occurred
+    - 'context' : line of code that issued the request that failed
+    """
+
+    def __init__(self, other=None):
+        """
+        Initializes a new instance, copying commands from other if it is not None
+
+        Args:
+            other: another instance of EnvironmentModifications from which (optional)
+        """
+        self.env_modifications = []
+        if other is not None:
+            self.extend(other)
+
+    def __iter__(self):
+        return iter(self.env_modifications)
+
+    def __len__(self):
+        return len(self.env_modifications)
+
+    def extend(self, other):
+        self._check_other(other)
+        self.env_modifications.extend(other.env_modifications)
+
+    @staticmethod
+    def _check_other(other):
+        if not isinstance(other, EnvironmentModifications):
+            raise TypeError('other must be an instance of EnvironmentModifications')
+
+    def _get_outside_caller_attributes(self):
+        stack = inspect.stack()
+        try:
+            _, filename, lineno, _, context, index = stack[2]
+            context = context[index].strip()
+        except Exception:
+            filename, lineno, context = 'unknown file', 'unknown line', 'unknown context'
+        args = {
+            'filename': filename,
+            'lineno': lineno,
+            'context': context
+        }
+        return args
+
+    def set_env(self, name, value, **kwargs):
+        """
+        Stores in the current object a request to set an environment variable
+
+        Args:
+            name: name of the environment variable to be set
+            value: value of the environment variable
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        item = SetEnv(name, value, **kwargs)
+        self.env_modifications.append(item)
+
+    def unset_env(self, name, **kwargs):
+        """
+        Stores in the current object a request to unset an environment variable
+
+        Args:
+            name: name of the environment variable to be set
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        item = UnsetEnv(name, **kwargs)
+        self.env_modifications.append(item)
+
+    def append_path(self, name, path, **kwargs):
+        """
+        Stores in the current object a request to append a path to a path list
+
+        Args:
+            name: name of the path list in the environment
+            path: path to be appended
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        item = AppendPath(name, path, **kwargs)
+        self.env_modifications.append(item)
+
+    def prepend_path(self, name, path, **kwargs):
+        """
+        Same as `append_path`, but the path is pre-pended
+
+        Args:
+            name: name of the path list in the environment
+            path: path to be pre-pended
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        item = PrependPath(name, path, **kwargs)
+        self.env_modifications.append(item)
+
+    def remove_path(self, name, path, **kwargs):
+        """
+        Stores in the current object a request to remove a path from a path list
+
+        Args:
+            name: name of the path list in the environment
+            path: path to be removed
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        item = RemovePath(name, path, **kwargs)
+        self.env_modifications.append(item)
+
+    def group_by_name(self):
+        """
+        Returns a dict of the modifications grouped by variable name
+
+        Returns:
+            dict mapping the environment variable name to the modifications to be done on it
+        """
+        modifications = collections.defaultdict(list)
+        for item in self:
+            modifications[item.name].append(item)
+        return modifications
+
+    def clear(self):
+        """
+        Clears the current list of modifications
+        """
+        self.env_modifications.clear()
+
+    def apply_modifications(self):
+        """
+        Applies the modifications and clears the list
+        """
+        modifications = self.group_by_name()
+        # Apply the modifications to the environment variables one variable at a time
+        for name, actions in sorted(modifications.items()):
+            for x in actions:
+                x.execute()
+
+
+def concatenate_paths(paths):
+    """
+    Concatenates an iterable of paths into a  string of column separated paths
+
+    Args:
+        paths: iterable of paths
+
+    Returns:
+        string
+    """
+    return ':'.join(str(item) for item in paths)
+
+
+def set_or_unset_not_first(variable, changes, errstream):
+    """
+    Check if we are going to set or unset something after other modifications have already been requested
+    """
+    indexes = [ii for ii, item in enumerate(changes) if ii != 0 and type(item) in [SetEnv, UnsetEnv]]
+    if indexes:
+        good = '\t    \t{context} at {filename}:{lineno}'
+        nogood = '\t--->\t{context} at {filename}:{lineno}'
+        errstream('Suspicious requests to set or unset the variable \'{var}\' found'.format(var=variable))
+        for ii, item in enumerate(changes):
+            print_format = nogood if ii in indexes else good
+            errstream(print_format.format(**item.args))
+
+
+def validate(env, errstream):
+    """
+    Validates the environment modifications to check for the presence of suspicious patterns. Prompts a warning for
+    everything that was found
+
+    Current checks:
+    - set or unset variables after other changes on the same variable
+
+    Args:
+        env: list of environment modifications
+    """
+    modifications = env.group_by_name()
+    for variable, list_of_changes in sorted(modifications.items()):
+        set_or_unset_not_first(variable, list_of_changes, errstream)

lib/spack/spack/modules.py

@@ -22,14 +22,12 @@
 # along with this program; if not, write to the Free Software Foundation,
 # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
-"""This module contains code for creating environment modules, which
+"""
-can include dotkits, tcl modules, lmod, and others.
+This module contains code for creating environment modules, which can include dotkits, tcl modules, lmod, and others.
 
-The various types of modules are installed by post-install hooks and
+The various types of modules are installed by post-install hooks and removed after an uninstall by post-uninstall hooks.
-removed after an uninstall by post-uninstall hooks.  This class
+This class consolidates the logic for creating an abstract description of the information that module systems need.
-consolidates the logic for creating an abstract description of the
+Currently that includes a number of directories to be appended to paths in the user's environment:
-information that module systems need.  Currently that includes a
-number of directories to be appended to paths in the user's environment:
 
   * /bin directories to be appended to PATH
   * /lib* directories for LD_LIBRARY_PATH
@@ -37,28 +35,25 @@
   * /man* and /share/man* directories for MANPATH
   * the package prefix for CMAKE_PREFIX_PATH
 
-This module also includes logic for coming up with unique names for
+This module also includes logic for coming up with unique names for the module files so that they can be found by the
-the module files so that they can be found by the various
+various shell-support files in $SPACK/share/spack/setup-env.*.
-shell-support files in $SPACK/share/spack/setup-env.*.
 
-Each hook in hooks/ implements the logic for writing its specific type
+Each hook in hooks/ implements the logic for writing its specific type of module file.
-of module file.
 """
-__all__ = ['EnvModule', 'Dotkit', 'TclModule']
-
 import os
+import os.path
 import re
-import textwrap
 import shutil
-from glob import glob
+import textwrap
 
 import llnl.util.tty as tty
-from llnl.util.filesystem import join_path, mkdirp
-
 import spack
+from llnl.util.filesystem import join_path, mkdirp
+from spack.environment import *
 
-"""Registry of all types of modules.  Entries created by EnvModule's
+__all__ = ['EnvModule', 'Dotkit', 'TclModule']
-   metaclass."""
+
+# Registry of all types of modules.  Entries created by EnvModule's metaclass
 module_types = {}
 
 
@@ -79,8 +74,43 @@ def print_help():
             "")
 
 
+def inspect_path(prefix):
+    """
+    Inspects the prefix of an installation to search for common layouts. Issues a request to modify the environment
+    accordingly when an item is found.
+
+    Args:
+        prefix: prefix of the installation
+
+    Returns:
+        instance of EnvironmentModifications containing the requested modifications
+    """
+    env = EnvironmentModifications()
+    # Inspect the prefix to check for the existence of common directories
+    prefix_inspections = {
+        'bin': ('PATH',),
+        'man': ('MANPATH',),
+        'lib': ('LIBRARY_PATH', 'LD_LIBRARY_PATH'),
+        'lib64': ('LIBRARY_PATH', 'LD_LIBRARY_PATH'),
+        'include': ('CPATH',)
+    }
+    for attribute, variables in prefix_inspections.items():
+        expected = getattr(prefix, attribute)
+        if os.path.isdir(expected):
+            for variable in variables:
+                env.prepend_path(variable, expected)
+    # PKGCONFIG
+    for expected in (join_path(prefix.lib, 'pkgconfig'), join_path(prefix.lib64, 'pkgconfig')):
+        if os.path.isdir(expected):
+            env.prepend_path('PKG_CONFIG_PATH', expected)
+    # CMake related variables
+    env.prepend_path('CMAKE_PREFIX_PATH', prefix)
+    return env
+
+
 class EnvModule(object):
     name = 'env_module'
+    formats = {}
 
     class __metaclass__(type):
         def __init__(cls, name, bases, dict):
@@ -88,67 +118,47 @@ def __init__(cls, name, bases, dict):
             if cls.name != 'env_module':
                 module_types[cls.name] = cls
 
-
     def __init__(self, spec=None):
-        # category in the modules system
-        # TODO: come up with smarter category names.
-        self.category = "spack"
-
-        # Descriptions for the module system's UI
-        self.short_description = ""
-        self.long_description = ""
-
-        # dict pathname -> list of directories to be prepended to in
-        # the module file.
-        self._paths = None
         self.spec = spec
+        self.pkg = spec.package  # Just stored for convenience
 
+        # short description default is just the package + version
+        # packages can provide this optional attribute
+        self.short_description = spec.format("$_ $@")
+        if hasattr(self.pkg, 'short_description'):
+            self.short_description = self.pkg.short_description
+
+        # long description is the docstring with reduced whitespace.
+        self.long_description = None
+        if self.spec.package.__doc__:
+            self.long_description = re.sub(r'\s+', ' ', self.spec.package.__doc__)
 
     @property
-    def paths(self):
+    def category(self):
-        if self._paths is None:
+        # Anything defined at the package level takes precedence
-            self._paths = {}
+        if hasattr(self.pkg, 'category'):
-
+            return self.pkg.category
-            def add_path(path_name, directory):
+        # Extensions
-                path = self._paths.setdefault(path_name, [])
+        for extendee in self.pkg.extendees:
-                path.append(directory)
+            return '{extendee} extension'.format(extendee=extendee)
-
+        # Not very descriptive fallback
-            # Add paths if they exist.
+        return 'spack installed package'
-            for var, directory in [
-                    ('PATH', self.spec.prefix.bin),
-                    ('MANPATH', self.spec.prefix.man),
-                    ('MANPATH', self.spec.prefix.share_man),
-                    ('LIBRARY_PATH', self.spec.prefix.lib),
-                    ('LIBRARY_PATH', self.spec.prefix.lib64),
-                    ('LD_LIBRARY_PATH', self.spec.prefix.lib),
-                    ('LD_LIBRARY_PATH', self.spec.prefix.lib64),
-                    ('CPATH', self.spec.prefix.include),
-                    ('PKG_CONFIG_PATH', join_path(self.spec.prefix.lib, 'pkgconfig')),
-                    ('PKG_CONFIG_PATH', join_path(self.spec.prefix.lib64, 'pkgconfig'))]:
-
-                if os.path.isdir(directory):
-                    add_path(var, directory)
-
-            # Add python path unless it's an actual python installation
-            # TODO: is there a better way to do this?
-            if self.spec.name != 'python':
-                site_packages = glob(join_path(self.spec.prefix.lib, "python*/site-packages"))
-                if site_packages:
-                    add_path('PYTHONPATH', site_packages[0])
-
-            if self.spec.package.extends(spack.spec.Spec('ruby')):
-              add_path('GEM_PATH', self.spec.prefix)
-
-            # short description is just the package + version
-            # TODO: maybe packages can optionally provide it.
-            self.short_description = self.spec.format("$_ $@")
-
-            # long description is the docstring with reduced whitespace.
-            if self.spec.package.__doc__:
-                self.long_description = re.sub(r'\s+', ' ', self.spec.package.__doc__)
-
-        return self._paths
 
+    # @property
+    # def paths(self):
+    #         # Add python path unless it's an actual python installation
+    #         # TODO : is there a better way to do this?
+    #         # FIXME : add PYTHONPATH to every python package
+    #         if self.spec.name != 'python':
+    #             site_packages = glob(join_path(self.spec.prefix.lib, "python*/site-packages"))
+    #             if site_packages:
+    #                 add_path('PYTHONPATH', site_packages[0])
+    #
+    #         # FIXME : Same for GEM_PATH
+    #         if self.spec.package.extends(spack.spec.Spec('ruby')):
+    #             add_path('GEM_PATH', self.spec.prefix)
+    #
+    #     return self._paths
 
     def write(self):
         """Write out a module file for this object."""
@@ -156,18 +166,36 @@ def write(self):
         if not os.path.exists(module_dir):
             mkdirp(module_dir)
 
-        # If there are no paths, no need for a dotkit.
+        # Environment modifications guessed by inspecting the installation prefix
-        if not self.paths:
+        env = inspect_path(self.spec.prefix)
+
+        # Let the extendee modify their extensions before asking for package-specific modifications
+        for extendee in self.pkg.extendees:
+            extendee_spec = self.spec[extendee]
+            extendee_spec.package.modify_module(self.pkg.module, extendee_spec, self.spec)
+
+        # Package-specific environment modifications
+        self.spec.package.setup_environment(env)
+        # TODO : implement site-specific modifications and filters
+        if not env:
             return
 
         with open(self.file_name, 'w') as f:
-            self._write(f)
+            self.write_header(f)
+            for line in self.process_environment_command(env):
+                f.write(line)
 
-
+    def write_header(self, stream):
-    def _write(self, stream):
-        """To be implemented by subclasses."""
         raise NotImplementedError()
 
+    def process_environment_command(self, env):
+        for command in env:
+            try:
+                yield self.formats[type(command)].format(**command.args)
+            except KeyError:
+                tty.warn('Cannot handle command of type {command} : skipping request'.format(command=type(command)))
+                tty.warn('{context} at {filename}:{lineno}'.format(**command.args))
+
 
     @property
     def file_name(self):
@@ -175,14 +203,12 @@ def file_name(self):
            where this module lives."""
         raise NotImplementedError()
 
-
     @property
     def use_name(self):
         """Subclasses should implement this to return the name the
            module command uses to refer to the package."""
         raise NotImplementedError()
 
-
     def remove(self):
         mod_file = self.file_name
         if os.path.exists(mod_file):
@@ -193,10 +219,14 @@ class Dotkit(EnvModule):
     name = 'dotkit'
     path = join_path(spack.share_path, "dotkit")
 
+    formats = {
+        PrependPath: 'dk_alter {name} {value}\n',
+        SetEnv: 'dk_setenv {name} {value}\n'
+    }
+
     @property
     def file_name(self):
-        return join_path(Dotkit.path, self.spec.architecture,
+        return join_path(Dotkit.path, self.spec.architecture, '%s.dk' % self.use_name)
-                         '%s.dk' % self.use_name)
 
     @property
     def use_name(self):
@@ -205,7 +235,7 @@ def use_name(self):
                                  self.spec.compiler.version, 
                                  self.spec.dag_hash())
 
-    def _write(self, dk_file):
+    def write_header(self, dk_file):
         # Category
         if self.category:
             dk_file.write('#c %s\n' % self.category)
@@ -219,24 +249,23 @@ def _write(self, dk_file):
             for line in textwrap.wrap(self.long_description, 72):
                 dk_file.write("#h %s\n" % line)
 
-        # Path alterations
-        for var, dirs in self.paths.items():
-            for directory in dirs:
-                dk_file.write("dk_alter %s %s\n" % (var, directory))
-
-        # Let CMake find this package.
-        dk_file.write("dk_alter CMAKE_PREFIX_PATH %s\n" % self.spec.prefix)
-
 
 class TclModule(EnvModule):
     name = 'tcl'
     path = join_path(spack.share_path, "modules")
 
+    formats = {
+        PrependPath: 'prepend-path {name} \"{value}\"\n',
+        AppendPath: 'append-path {name} \"{value}\"\n',
+        RemovePath: 'remove-path {name} \"{value}\"\n',
+        SetEnv: 'setenv {name} \"{value}\"\n',
+        UnsetEnv: 'unsetenv {name}\n'
+    }
+
     @property
     def file_name(self):
         return join_path(TclModule.path, self.spec.architecture, self.use_name)
 
-
     @property
     def use_name(self):
       return "%s-%s-%s-%s-%s" % (self.spec.name, self.spec.version,
@@ -244,25 +273,17 @@ def use_name(self):
                                  self.spec.compiler.version, 
                                  self.spec.dag_hash())
 
-
+    def write_header(self, module_file):
-    def _write(self, m_file):
+        # TCL Modulefile header
-        # TODO: cateogry?
+        module_file.write('#%Module1.0\n')
-        m_file.write('#%Module1.0\n')
+        # TODO : category ?
-
         # Short description
         if self.short_description:
-            m_file.write('module-whatis \"%s\"\n\n' % self.short_description)
+            module_file.write('module-whatis \"%s\"\n\n' % self.short_description)
 
         # Long description
         if self.long_description:
-            m_file.write('proc ModulesHelp { } {\n')
+            module_file.write('proc ModulesHelp { } {\n')
             doc = re.sub(r'"', '\"', self.long_description)
-            m_file.write("puts stderr \"%s\"\n" % doc)
+            module_file.write("puts stderr \"%s\"\n" % doc)
-            m_file.write('}\n\n')
+            module_file.write('}\n\n')
-
-        # Path alterations
-        for var, dirs in self.paths.items():
-            for directory in dirs:
-                m_file.write("prepend-path %s \"%s\"\n" % (var, directory))
-
-        m_file.write("prepend-path CMAKE_PREFIX_PATH \"%s\"\n" % self.spec.prefix)

lib/spack/spack/package.py

@@ -34,40 +34,34 @@
 README.
 """
 import os
-import errno
 import re
-import shutil
-import time
-import itertools
-import subprocess
-import platform as py_platform
-import multiprocessing
-from urlparse import urlparse, urljoin
 import textwrap
-from StringIO import StringIO
+import time
+import glob
 
 import llnl.util.tty as tty
-from llnl.util.tty.log import log_output
-from llnl.util.link_tree import LinkTree
-from llnl.util.filesystem import *
-from llnl.util.lang import *
-
 import spack
-import spack.error
-import spack.compilers
-import spack.mirror
-import spack.hooks
-import spack.directives
-import spack.repository
 import spack.build_environment
+import spack.compilers
+import spack.directives
+import spack.error
+import spack.fetch_strategy as fs
+import spack.hooks
+import spack.mirror
+import spack.repository
 import spack.url
 import spack.util.web
-import spack.fetch_strategy as fs
+from StringIO import StringIO
-from spack.version import *
+from llnl.util.filesystem import *
+from llnl.util.lang import *
+from llnl.util.link_tree import LinkTree
+from llnl.util.tty.log import log_output
 from spack.stage import Stage, ResourceStage, StageComposite
-from spack.util.compression import allowed_archive, extension
+from spack.util.compression import allowed_archive
-from spack.util.executable import ProcessError
 from spack.util.environment import dump_environment
+from spack.util.executable import ProcessError
+from spack.version import *
+from urlparse import urlparse
 
 """Allowed URL schemes for spack packages."""
 _ALLOWED_URL_SCHEMES = ["http", "https", "ftp", "file", "git"]
@@ -1008,38 +1002,63 @@ def module(self):
         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_dependent_environment(self, module, spec, dependent_spec):
+        Default implementation does nothing, but this can be overridden if the package needs a particular environment.
-        """Called before the install() method of dependents.
 
-        Default implementation does nothing, but this can be
+        Example :
-        overridden by an extendable package to set up the install
-        environment for its extensions.  This is useful if there are
-        some common steps to installing all extensions for a
-        certain package.
 
-        Some examples:
+        1. A lot of Qt extensions need `QTDIR` set.  This can be used to do that.
-
-        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.
-
-        2. Extensions often need to invoke the 'python' interpreter
-           from the Python installation being extended.  This routine can
-           put a 'python' Execuable object in the module scope for the
-           extension package to simplify extension installs.
-
-        3. A lot of Qt extensions need QTDIR set.  This can be used to do that.
 
+        Args:
+            env: list of environment modifications to be updated
         """
         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.
+
+        This is useful if there are some common steps to installing all extensions for a certain package.
+
+        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.
+
+        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):
+        """
+        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.
+
+        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.
+        """
+        pass
 
     def install(self, spec, prefix):
         """Package implementations override this with their own build configuration."""
         raise InstallError("Package %s provides no install method!" % self.name)
 
-
     def do_uninstall(self, force=False):
         if not self.installed:
             raise InstallError(str(self.spec) + " is not installed.")

lib/spack/spack/test/__init__.py

@@ -66,7 +66,8 @@
               'database',
               'namespace_trie',
               'yaml',
-              'sbang']
+              'sbang',
+              'environment']
 
 
 def list_tests():

lib/spack/spack/test/database.py

@@ -26,6 +26,7 @@
 These tests check the database is functioning properly,
 both in memory and in its file
 """
+import os.path
 import multiprocessing
 import shutil
 import tempfile

lib/spack/spack/test/environment.py

@@ -0,0 +1,67 @@
+import unittest
+import os
+from spack.environment import EnvironmentModifications
+
+
+class EnvironmentTest(unittest.TestCase):
+    def setUp(self):
+        os.environ.clear()
+        os.environ['UNSET_ME'] = 'foo'
+        os.environ['EMPTY_PATH_LIST'] = ''
+        os.environ['PATH_LIST'] = '/path/second:/path/third'
+        os.environ['REMOVE_PATH_LIST'] = '/a/b:/duplicate:/a/c:/remove/this:/a/d:/duplicate/:/f/g'
+
+    def test_set_env(self):
+        env = EnvironmentModifications()
+        env.set_env('A', 'dummy value')
+        env.set_env('B', 3)
+        env.apply_modifications()
+        self.assertEqual('dummy value', os.environ['A'])
+        self.assertEqual(str(3), os.environ['B'])
+
+    def test_unset_env(self):
+        env = EnvironmentModifications()
+        self.assertEqual('foo', os.environ['UNSET_ME'])
+        env.unset_env('UNSET_ME')
+        env.apply_modifications()
+        self.assertRaises(KeyError, os.environ.__getitem__, 'UNSET_ME')
+
+    def test_path_manipulation(self):
+        env = EnvironmentModifications()
+
+        env.append_path('PATH_LIST', '/path/last')
+        env.prepend_path('PATH_LIST', '/path/first')
+
+        env.append_path('EMPTY_PATH_LIST', '/path/middle')
+        env.append_path('EMPTY_PATH_LIST', '/path/last')
+        env.prepend_path('EMPTY_PATH_LIST', '/path/first')
+
+        env.append_path('NEWLY_CREATED_PATH_LIST', '/path/middle')
+        env.append_path('NEWLY_CREATED_PATH_LIST', '/path/last')
+        env.prepend_path('NEWLY_CREATED_PATH_LIST', '/path/first')
+
+        env.remove_path('REMOVE_PATH_LIST', '/remove/this')
+        env.remove_path('REMOVE_PATH_LIST', '/duplicate/')
+
+        env.apply_modifications()
+        self.assertEqual('/path/first:/path/second:/path/third:/path/last', os.environ['PATH_LIST'])
+        self.assertEqual('/path/first:/path/middle:/path/last', os.environ['EMPTY_PATH_LIST'])
+        self.assertEqual('/path/first:/path/middle:/path/last', os.environ['NEWLY_CREATED_PATH_LIST'])
+        self.assertEqual('/a/b:/a/c:/a/d:/f/g', os.environ['REMOVE_PATH_LIST'])
+
+    def test_extra_arguments(self):
+        env = EnvironmentModifications()
+        env.set_env('A', 'dummy value', who='Pkg1')
+        for x in env:
+            assert 'who' in x.args
+        env.apply_modifications()
+        self.assertEqual('dummy value', os.environ['A'])
+
+    def test_extend(self):
+        env = EnvironmentModifications()
+        env.set_env('A', 'dummy value')
+        env.set_env('B', 3)
+        copy_construct = EnvironmentModifications(env)
+        self.assertEqual(len(copy_construct), 2)
+        for x, y in zip(env, copy_construct):
+            assert x is y

lib/spack/spack/util/environment.py

@@ -59,14 +59,8 @@ def path_put_first(var_name, directories):
     path_set(var_name, new_path)
 
 
-def pop_keys(dictionary, *keys):
-    for key in keys:
-        if key in dictionary:
-            dictionary.pop(key)
-
-
 def dump_environment(path):
     """Dump the current environment out to a file."""
     with open(path, 'w') as env_file:
-        for key,val in sorted(os.environ.items()):
+        for key, val in sorted(os.environ.items()):
             env_file.write("%s=%s\n" % (key, val))

var/spack/repos/builtin/packages/mpich/package.py

@@ -25,6 +25,7 @@
 from spack import *
 import os
 
+
 class Mpich(Package):
     """MPICH is a high performance and widely portable implementation of
        the Message Passing Interface (MPI) standard."""
@@ -46,14 +47,23 @@ class Mpich(Package):
     provides('mpi@:3.0', when='@3:')
     provides('mpi@:1.3', when='@1:')
 
-    def setup_dependent_environment(self, module, spec, dep_spec):
+    def setup_environment(self, env):
-        """For dependencies, make mpicc's use spack wrapper."""
+        env.set_env('MPICH_CC', self.compiler.cc)
-        os.environ['MPICH_CC']  = os.environ['CC']
+        env.set_env('MPICH_CXX', self.compiler.cxx)
-        os.environ['MPICH_CXX'] = os.environ['CXX']
+        env.set_env('MPICH_F77', self.compiler.f77)
-        os.environ['MPICH_F77'] = os.environ['F77']
+        env.set_env('MPICH_F90', self.compiler.fc)
-        os.environ['MPICH_F90'] = os.environ['FC']
+        env.set_env('MPICH_FC', self.compiler.fc)
-        os.environ['MPICH_FC'] = os.environ['FC']
 
+    def setup_dependent_environment(self, env, dependent_spec):
+        env.set_env('MPICH_CC', spack_cc)
+        env.set_env('MPICH_CXX', spack_cxx)
+        env.set_env('MPICH_F77', spack_f77)
+        env.set_env('MPICH_F90', spack_f90)
+        env.set_env('MPICH_FC', spack_fc)
+
+    def modify_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')
 
     def install(self, spec, prefix):

var/spack/repos/builtin/packages/mvapich2/package.py

@@ -123,7 +123,7 @@ def set_network_type(self, spec, configure_args):
                 count += 1
         if count > 1:
             raise RuntimeError('network variants are mutually exclusive (only one can be selected at a time)')
-
+        network_options = []
         # From here on I can suppose that only one variant has been selected
         if self.enabled(Mvapich2.PSM) in spec:
             network_options = ["--with-device=ch3:psm"]

var/spack/repos/builtin/packages/netlib-scalapack/package.py

@@ -1,5 +1,4 @@
 from spack import *
-import sys
 
 class NetlibScalapack(Package):
     """ScaLAPACK is a library of high-performance linear algebra routines for parallel distributed memory machines"""
@@ -41,11 +40,10 @@ def install(self, spec, prefix):
             make()
             make("install")
 
-    def setup_dependent_environment(self, module, spec, dependent_spec):
+    def modify_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'
 
         spec['scalapack'].fc_link = '-L%s -lscalapack' % spec['scalapack'].prefix.lib
         spec['scalapack'].cc_link = spec['scalapack'].fc_link
-        spec['scalapack'].libraries = [join_path(spec['scalapack'].prefix.lib,
+        spec['scalapack'].libraries = [join_path(spec['scalapack'].prefix.lib, 'libscalapack%s' % lib_suffix)]
-                                                 'libscalapack%s' % lib_suffix)]

var/spack/repos/builtin/packages/openmpi/package.py

@@ -41,12 +41,17 @@ 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_dependent_environment(self, module, spec, dep_spec):
+    def setup_environment(self, env):
-        """For dependencies, make mpicc's use spack wrapper."""
+        env.set_env('OMPI_CC', self.compiler.cc)
-        os.environ['OMPI_CC'] = 'cc'
+        env.set_env('OMPI_CXX', self.compiler.cxx)
-        os.environ['OMPI_CXX'] = 'c++'
+        env.set_env('OMPI_FC', self.compiler.fc)
-        os.environ['OMPI_FC'] = 'f90'
+        env.set_env('OMPI_F77', self.compiler.f77)
-        os.environ['OMPI_F77'] = '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 install(self, spec, prefix):
         config_args = ["--prefix=%s" % prefix,

var/spack/repos/builtin/packages/py-nose/package.py

@@ -1,11 +1,12 @@
 from spack import *
 
+
 class PyNose(Package):
     """nose extends the test loading and running features of unittest,
     making it easier to write, find and run tests."""
 
     homepage = "https://pypi.python.org/pypi/nose"
-    url      = "https://pypi.python.org/packages/source/n/nose/nose-1.3.4.tar.gz"
+    url = "https://pypi.python.org/packages/source/n/nose/nose-1.3.4.tar.gz"
 
     version('1.3.4', '6ed7169887580ddc9a8e16048d38274d')
     version('1.3.6', '0ca546d81ca8309080fc80cb389e7a16')

var/spack/repos/builtin/packages/python/package.py

@@ -1,11 +1,14 @@
+import functools
+import glob
+import inspect
 import os
 import re
 from contextlib import closing
-from llnl.util.lang import match_predicate
-from spack.util.environment import *
 
-from spack import *
 import spack
+from llnl.util.lang import match_predicate
+from spack import *
+from spack.util.environment import *
 
 
 class Python(Package):
@@ -89,13 +92,21 @@ def python_include_dir(self):
     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.
+        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))
 
-    def setup_dependent_environment(self, module, spec, ext_spec):
+    def modify_module(self, module, spec, ext_spec):
-        """Called before python modules' install() methods.
+        """
+        Called before python modules' install() methods.
 
         In most cases, extensions will only need to have one line::
 
-            python('setup.py', 'install', '--prefix=%s' % prefix)
+        python('setup.py', 'install', '--prefix=%s' % prefix)
         """
         # Python extension builds can have a global python executable function
         if self.version >= Version("3.0.0") and self.version < Version("4.0.0"):
@@ -103,6 +114,31 @@ def setup_dependent_environment(self, module, spec, ext_spec):
         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)
@@ -111,15 +147,6 @@ def setup_dependent_environment(self, module, spec, ext_spec):
         # Make the site packages directory if it does not exist already.
         mkdirp(module.site_packages_dir)
 
-        # Set PYTHONPATH to include site-packages dir for the
-        # extension and any other python extensions it depends on.
-        python_paths = []
-        for d in ext_spec.traverse():
-            if d.package.extends(self.spec):
-                python_paths.append(os.path.join(d.prefix, self.site_packages_dir))
-        os.environ['PYTHONPATH'] = ':'.join(python_paths)
-
-
     # ========================================================================
     # Handle specifics of activating and deactivating python modules.
     # ========================================================================

var/spack/repos/builtin/packages/qt/package.py

@@ -55,11 +55,8 @@ class Qt(Package):
     depends_on("mesa", when='@4:+mesa')
     depends_on("libxcb")
 
-
+    def setup_environment(self, env):
-    def setup_dependent_environment(self, module, spec, dep_spec):
+        env.set_env['QTDIR'] = self.prefix
-        """Dependencies of Qt find it using the QTDIR environment variable."""
-        os.environ['QTDIR'] = self.prefix
-
 
     def patch(self):
         if self.spec.satisfies('@4'):

var/spack/repos/builtin/packages/ruby/package.py

@@ -1,6 +1,5 @@
 from spack import *
-import spack
+
-import os
 
 class Ruby(Package):
     """A dynamic, open source programming language with a focus on 
@@ -15,11 +14,20 @@ class Ruby(Package):
 
     def install(self, spec, prefix):
         configure("--prefix=%s" % prefix)
-
         make()
         make("install")
 
-    def setup_dependent_environment(self, module, spec, ext_spec):
+    def setup_dependent_environment(self, env, extension_spec):
+        # 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))
+        # The actual installation path for this gem
+        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
         and GEM_PATH to values appropriate for the package being built.
 
@@ -31,11 +39,4 @@ def setup_dependent_environment(self, module, spec, ext_spec):
         module.ruby = Executable(join_path(spec.prefix.bin, 'ruby'))
         module.gem = Executable(join_path(spec.prefix.bin, 'gem'))
 
-        # Set GEM_PATH to include dependent gem directories
+
-        ruby_paths = []
-        for d in ext_spec.traverse():
-            if d.package.extends(self.spec):
-                ruby_paths.append(d.prefix)
-        os.environ['GEM_PATH'] = ':'.join(ruby_paths)
-        # The actual installation path for this gem
-        os.environ['GEM_HOME'] = ext_spec.prefix