environment : added test, modified docs

4 files changed, 79 insertions, 43 deletions

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 18fef1ef12..5a68e10c99 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -3,33 +3,40 @@ import os.path
 import collections
 
 
-class SetEnv(object):
+class AttributeHolder(object):
+    """
+    Policy that permits to store any kind of attribute on self. The attributes must be passed as key/value pairs
+    during the initialization of the instance.
+    """
+    def __init__(self, **kwargs):
+        for key, value in kwargs.items():
+            setattr(self, key, value)
+
+
+class SetEnv(AttributeHolder):
     def __init__(self, name, value, **kwargs):
+        super(SetEnv, self).__init__(**kwargs)
         self.name = name
         self.value = value
-        for key, value in kwargs.items():
-            setattr(self, key, value)
 
     def execute(self):
         os.environ[self.name] = str(self.value)
 
 
-class UnsetEnv(object):
+class UnsetEnv(AttributeHolder):
     def __init__(self, name, **kwargs):
+        super(UnsetEnv, self).__init__(**kwargs)
         self.name = name
-        for key, value in kwargs.items():
-            setattr(self, key, value)
 
     def execute(self):
         os.environ.pop(self.name, None)  # Avoid throwing if the variable was not set
 
 
-class AppendPath(object):
+class AppendPath(AttributeHolder):
     def __init__(self, name, path, **kwargs):
+        super(AppendPath, self).__init__(**kwargs)
         self.name = name
         self.path = path
-        for key, value in kwargs.items():
-            setattr(self, key, value)
 
     def execute(self):
         environment_value = os.environ.get(self.name, '')
@@ -39,12 +46,11 @@ class AppendPath(object):
         os.environ[self.name] = ':'.join(directories)
 
 
-class PrependPath(object):
+class PrependPath(AttributeHolder):
     def __init__(self, name, path, **kwargs):
+        super(PrependPath, self).__init__(**kwargs)
         self.name = name
         self.path = path
-        for key, value in kwargs.items():
-            setattr(self, key, value)
 
     def execute(self):
         environment_value = os.environ.get(self.name, '')
@@ -54,12 +60,11 @@ class PrependPath(object):
         os.environ[self.name] = ':'.join(directories)
 
 
-class RemovePath(object):
+class RemovePath(AttributeHolder):
     def __init__(self, name, path, **kwargs):
+        super(RemovePath, self).__init__(**kwargs)
         self.name = name
         self.path = path
-        for key, value in kwargs.items():
-            setattr(self, key, value)
 
     def execute(self):
         environment_value = os.environ.get(self.name, '')
@@ -70,18 +75,26 @@ class RemovePath(object):
 
 class EnvironmentModifications(object):
     """
-    Keeps track of requests to modify the current environment
+    Keeps track of requests to modify the current environment.
     """
 
     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._check_other(other)
-            self.env_modifications.extend(other.env_modifications)
+            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)
@@ -147,8 +160,18 @@ class EnvironmentModifications(object):
 
 
 def concatenate_paths(paths):
+    """
+    Concatenates an iterable of paths into a column separated string
+
+    Args:
+        paths: iterable of paths
+
+    Returns:
+        column separated string
+    """
     return ':'.join(str(item) for item in paths)
 
+
 def validate_environment_modifications(env):
     modifications = collections.defaultdict(list)
     for item in env:
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index c1a5c912be..5d8258d4cf 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -985,30 +985,41 @@ class Package(object):
 
 
     def environment_modifications(self, dependent_spec):
-        return EnvironmentModifications()
+        """
+        Called before the install() method of dependents.
 
-    def module_modifications(self, module, spec, dependent_spec):
-        """Called before the install() method of dependents.
+        Return the list of environment modifications needed by dependents (or extensions). Default implementation does
+        nothing, but this can be 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.
 
-        Default implementation does nothing, but this can be
-        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.
+        Example :
 
-        Some examples:
+        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.
+        2. A lot of Qt extensions need `QTDIR` set.  This can be used to do that.
 
-        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.
+        Args:
+            dependent_spec: dependent (or extension) of this spec
+
+        Returns:
+            instance of environment modifications
+        """
+        return EnvironmentModifications()
+
+    def module_modifications(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.
 
-        3. A lot of Qt extensions need QTDIR set.  This can be used to do that.
+        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
 
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index 17061c8fd0..97581ecb76 100644
--- a/lib/spack/spack/test/environment.py
+++ b/lib/spack/spack/test/environment.py
@@ -55,5 +55,11 @@ class EnvironmentTest(unittest.TestCase):
         apply_environment_modifications(env)
         self.assertEqual('dummy value', os.environ['A'])
 
-    def test_copy(self):
-        pass
+    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):
+            self.assertIs(x, y)
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index ae8e5708be..00cda8d508 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -40,11 +40,13 @@ def env_flag(name):
     return False
 
 
+# FIXME : remove this function ?
 def path_set(var_name, directories):
     path_str = ":".join(str(dir) for dir in directories)
     os.environ[var_name] = path_str
 
 
+# FIXME : remove this function ?
 def path_put_first(var_name, directories):
     """Puts the provided directories first in the path, adding them
        if they're not already there.
@@ -59,12 +61,6 @@ 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: