environment : simplified modification of the environment

5 files changed, 46 insertions, 40 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index e86a7c413a..e5d256a2e0 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -35,7 +35,7 @@ import sys
 
 import spack
 from llnl.util.filesystem import *
-from spack.environment import EnvironmentModifications, apply_environment_modifications, concatenate_paths
+from spack.environment import EnvironmentModifications, concatenate_paths
 from spack.util.environment import *
 from spack.util.executable import Executable, which
 
@@ -283,10 +283,12 @@ def setup_package(pkg):
         set_module_variables_for_package(pkg, mod)
 
     # Allow dependencies to set up environment as well.
-    for dep_spec in pkg.spec.traverse(root=False):
-        dep_spec.package.module_modifications(pkg.module, dep_spec, pkg.spec)
-        env.extend(dep_spec.package.environment_modifications(pkg.spec))
-    apply_environment_modifications(env)
+    for dependency_spec in pkg.spec.traverse(root=False):
+        dependency_spec.package.module_modifications(pkg.module, dependency_spec, pkg.spec)
+        env.extend(dependency_spec.package.environment_modifications(pkg.spec))
+    # TODO : implement validation
+    #validate(env)
+    env.apply_modifications()
 
 
 def fork(pkg, function):
diff --git a/lib/spack/spack/cmd/module.py b/lib/spack/spack/cmd/module.py
index 1d6867c1d9..315d9fc926 100644
--- a/lib/spack/spack/cmd/module.py
+++ b/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():
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 5a68e10c99..b557bf0bbc 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -158,41 +158,43 @@ class EnvironmentModifications(object):
         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 column separated string
+    Concatenates an iterable of paths into a  string of column separated paths
 
     Args:
         paths: iterable of paths
 
     Returns:
-        column separated string
+        string
     """
     return ':'.join(str(item) for item in paths)
-
-
-def validate_environment_modifications(env):
-    modifications = collections.defaultdict(list)
-    for item in env:
-        modifications[item.name].append(item)
-    # TODO : once we organized the modifications into a dictionary that maps an environment variable
-    # TODO : to a list of action to be done on it, we may easily spot inconsistencies and warn the user if
-    # TODO : something suspicious is happening
-    return modifications
-
-
-def apply_environment_modifications(env):
-    """
-    Modifies the current environment according to the request in env
-
-    Args:
-        env: object storing modifications to the environment
-    """
-    modifications = validate_environment_modifications(env)
-
-    # Cycle over the environment variables that will be modified
-    for variable, actions in modifications.items():
-        # Execute all the actions in the order they were issued
-        for x in actions:
-            x.execute()
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index f44282d6f3..d0b093b054 100644
--- a/lib/spack/spack/test/environment.py
+++ b/lib/spack/spack/test/environment.py
@@ -1,6 +1,6 @@
 import unittest
 import os
-from spack.environment import EnvironmentModifications, apply_environment_modifications
+from spack.environment import EnvironmentModifications
 
 
 class EnvironmentTest(unittest.TestCase):
@@ -15,7 +15,7 @@ class EnvironmentTest(unittest.TestCase):
         env = EnvironmentModifications()
         env.set_env('A', 'dummy value')
         env.set_env('B', 3)
-        apply_environment_modifications(env)
+        env.apply_modifications()
         self.assertEqual('dummy value', os.environ['A'])
         self.assertEqual(str(3), os.environ['B'])
 
@@ -23,7 +23,7 @@ class EnvironmentTest(unittest.TestCase):
         env = EnvironmentModifications()
         self.assertEqual('foo', os.environ['UNSET_ME'])
         env.unset_env('UNSET_ME')
-        apply_environment_modifications(env)
+        env.apply_modifications()
         self.assertRaises(KeyError, os.environ.__getitem__, 'UNSET_ME')
 
     def test_path_manipulation(self):
@@ -43,7 +43,7 @@ class EnvironmentTest(unittest.TestCase):
         env.remove_path('REMOVE_PATH_LIST', '/remove/this')
         env.remove_path('REMOVE_PATH_LIST', '/duplicate/')
 
-        apply_environment_modifications(env)
+        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'])
@@ -52,7 +52,9 @@ class EnvironmentTest(unittest.TestCase):
     def test_extra_arguments(self):
         env = EnvironmentModifications()
         env.set_env('A', 'dummy value', who='Pkg1')
-        apply_environment_modifications(env)
+        for x in env:
+            assert hasattr(x, 'who')
+        env.apply_modifications()
         self.assertEqual('dummy value', os.environ['A'])
 
     def test_extend(self):
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index 00cda8d508..1485992b0f 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -64,5 +64,5 @@ def path_put_first(var_name, directories):
 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))