env activation: use package defined env setup methods (#13249)

This PR ensures that environment activation sets all environment variables set by the equivalent `module load` operations, except that the spec prefixes are "rebased" to the view associated with the environment.

Currently, Spack blindly adds paths relative to the environment view root to the user environment on activation. Issue #12731 points out ways in which this behavior is insufficient.

This PR changes that behavior to use the `setup_run_environment` logic for each package to augment the prefix inspections (as in Spack's modulefile generation logic) to ensure that all necessary variables are set to make use of the packages in the environment.

See #12731 for details on the previous problems in behavior.

This PR also updates the `ViewDescriptor` object in `spack.environment` to have a `__contains__` method. This allows for checks like `if spec in self.default_view`. The `__contains__` operator for `ViewDescriptor` objects checks whether the spec satisfies the filters of the View descriptor, not whether the spec is already linked into the underlying `FilesystemView` object.

6 files changed, 191 insertions, 46 deletions

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index ab8d7e14f1..5204f7d288 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -28,9 +28,11 @@ import spack.schema.env
 import spack.spec
 import spack.util.spack_json as sjson
 import spack.config
+import spack.build_environment as build_env
 
+from spack.util.prefix import Prefix
 from spack.filesystem_view import YamlFilesystemView
-from spack.util.environment import EnvironmentModifications
+import spack.util.environment
 import spack.architecture as architecture
 from spack.spec import Spec
 from spack.spec_list import SpecList, InvalidSpecConstraintError
@@ -468,6 +470,23 @@ class ViewDescriptor(object):
                                   ignore_conflicts=True,
                                   projections=self.projections)
 
+    def __contains__(self, spec):
+        """Is the spec described by the view descriptor
+
+        Note: This does not claim the spec is already linked in the view.
+        It merely checks that the spec is selected if a select operation is
+        specified and is not excluded if an exclude operator is specified.
+        """
+        if self.select:
+            if not self.select_fn(spec):
+                return False
+
+        if self.exclude:
+            if not self.exclude_fn(spec):
+                return False
+
+        return True
+
     def regenerate(self, all_specs, roots):
         specs_for_view = []
         specs = all_specs if self.link == 'all' else roots
@@ -478,14 +497,8 @@ class ViewDescriptor(object):
             if spec.concrete:  # Do not link unconcretized roots
                 specs_for_view.append(spec.copy(deps=('link', 'run')))
 
-        if self.select:
-            specs_for_view = list(filter(self.select_fn, specs_for_view))
-
-        if self.exclude:
-            specs_for_view = list(filter(self.exclude_fn, specs_for_view))
-
         installed_specs_for_view = set(s for s in specs_for_view
-                                       if s.package.installed)
+                                       if s in self and s.package.installed)
 
         view = self.view()
 
@@ -1009,38 +1022,76 @@ class Environment(object):
         for view in self.views.values():
             view.regenerate(specs, self.roots())
 
-    def _shell_vars(self):
-        updates = [
-            ('PATH', ['bin']),
-            ('MANPATH', ['man', 'share/man']),
-            ('ACLOCAL_PATH', ['share/aclocal']),
-            ('LD_LIBRARY_PATH', ['lib', 'lib64']),
-            ('LIBRARY_PATH', ['lib', 'lib64']),
-            ('CPATH', ['include']),
-            ('PKG_CONFIG_PATH', ['lib/pkgconfig', 'lib64/pkgconfig',
-                                 'share/pkgconfig']),
-            ('CMAKE_PREFIX_PATH', ['']),
-        ]
-
-        path_updates = list()
-        if default_view_name in self.views:
-            for var, dirs in updates:
-                paths = [os.path.join(self.default_view.root, x) for x in dirs]
-                path_updates.append((var, paths))
-        return path_updates
+    prefix_inspections = {
+        'bin': ['PATH'],
+        'lib': ['LD_LIBRARY_PATH', 'LIBRARY_PATH', 'DYLD_LIBRARY_PATH'],
+        'lib64': ['LD_LIBRARY_PATH', 'LIBRARY_PATH', 'DYLD_LIBRARY_PATH'],
+        'man': ['MANPATH'],
+        'share/man': ['MANPATH'],
+        'share/aclocal': ['ACLOCAL_PATH'],
+        'include': ['CPATH'],
+        'lib/pkgconfig': ['PKG_CONFIG_PATH'],
+        'lib64/pkgconfig': ['PKG_CONFIG_PATH'],
+        '': ['CMAKE_PREFIX_PATH']
+    }
+
+    def environment_modifications_for_spec(self, spec, view=None):
+        """List of environment modifications to be processed."""
+        spec = spec.copy()
+        if view:
+            spec.prefix = Prefix(view.view().get_projection_for_spec(spec))
+
+        # generic environment modifications determined by inspecting the spec
+        # prefix
+        env = spack.util.environment.inspect_path(
+            spec.prefix,
+            self.prefix_inspections,
+            exclude=spack.util.environment.is_system_path
+        )
+
+        # Let the extendee/dependency modify their extensions/dependents
+        # before asking for package-specific modifications
+        env.extend(
+            build_env.modifications_from_dependencies(
+                spec, context='run'
+            )
+        )
+        # Package specific modifications
+        build_env.set_module_variables_for_package(spec.package)
+        spec.package.setup_run_environment(env)
+
+        return env
 
     def add_default_view_to_shell(self, shell):
-        env_mod = EnvironmentModifications()
-        for var, paths in self._shell_vars():
-            for path in paths:
-                env_mod.prepend_path(var, path)
+        env_mod = spack.util.environment.EnvironmentModifications()
+
+        if default_view_name not in self.views:
+            # No default view to add to shell
+            return env_mod.shell_modifications(shell)
+
+        for _, spec in self.concretized_specs():
+            if spec in self.default_view:
+                env_mod.extend(self.environment_modifications_for_spec(
+                    spec, self.default_view))
+
+        # deduplicate paths from specs mapped to the same location
+        for env_var in env_mod.group_by_name():
+            env_mod.prune_duplicate_paths(env_var)
+
         return env_mod.shell_modifications(shell)
 
     def rm_default_view_from_shell(self, shell):
-        env_mod = EnvironmentModifications()
-        for var, paths in self._shell_vars():
-            for path in paths:
-                env_mod.remove_path(var, path)
+        env_mod = spack.util.environment.EnvironmentModifications()
+
+        if default_view_name not in self.views:
+            # No default view to add to shell
+            return env_mod.shell_modifications(shell)
+
+        for _, spec in self.concretized_specs():
+            if spec in self.default_view:
+                env_mod.extend(
+                    self.environment_modifications_for_spec(
+                        spec, self.default_view).reversed())
         return env_mod.shell_modifications(shell)
 
     def _add_concrete_spec(self, spec, concrete, new=True):
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 232da811a3..f2912f3ec0 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -1577,8 +1577,10 @@ env:
             install()
 
         shell = env('activate', '--sh', 'test')
+
         assert 'PATH' in shell
         assert os.path.join(viewdir, 'bin') in shell
+        assert 'FOOBAR=mpileaks' in shell
 
 
 def test_stack_view_no_activate_without_default(tmpdir, mock_fetch,
diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index f83a2dbb8e..290d21b85f 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -172,15 +172,16 @@ def test_prs_update_old_api():
     ]
     failing = []
     for file in changed_package_files:
-        name = os.path.basename(os.path.dirname(file))
-        pkg = spack.repo.get(name)
+        if 'builtin.mock' not in file:  # don't restrict packages for tests
+            name = os.path.basename(os.path.dirname(file))
+            pkg = spack.repo.get(name)
+
+            failed = (hasattr(pkg, 'setup_environment') or
+                      hasattr(pkg, 'setup_dependent_environment'))
+            if failed:
+                failing.append(name)
 
-        # Check for old APIs
-        failed = (hasattr(pkg, 'setup_environment') or
-                  hasattr(pkg, 'setup_dependent_environment'))
-        if failed:
-            failing.append(pkg)
     msg = 'there are {0} packages still using old APIs in this PR [{1}]'
     assert not failing, msg.format(
-        len(failing), ','.join(x.name for x in failing)
+        len(failing), ','.join(failing)
     )
diff --git a/lib/spack/spack/test/util/environment.py b/lib/spack/spack/test/util/environment.py
index cb1748fa28..e34fc95f33 100644
--- a/lib/spack/spack/test/util/environment.py
+++ b/lib/spack/spack/test/util/environment.py
@@ -103,3 +103,34 @@ def test_dump_environment(prepare_environment_for_tests, tmpdir):
     with open(dumpfile_path, 'r') as dumpfile:
         assert('TEST_ENV_VAR={0}; export TEST_ENV_VAR\n'.format(test_paths)
                in list(dumpfile))
+
+
+def test_reverse_environment_modifications(working_env):
+    start_env = {
+        'PREPEND_PATH': '/path/to/prepend/to',
+        'APPEND_PATH': '/path/to/append/to',
+        'UNSET': 'var_to_unset',
+        'APPEND_FLAGS': 'flags to append to',
+    }
+
+    to_reverse = envutil.EnvironmentModifications()
+
+    to_reverse.prepend_path('PREPEND_PATH', '/new/path/prepended')
+    to_reverse.append_path('APPEND_PATH', '/new/path/appended')
+    to_reverse.set_path('SET_PATH', ['/one/set/path', '/two/set/path'])
+    to_reverse.set('SET', 'a var')
+    to_reverse.unset('UNSET')
+    to_reverse.append_flags('APPEND_FLAGS', 'more_flags')
+
+    reversal = to_reverse.reversed()
+
+    os.environ = start_env.copy()
+
+    print(os.environ)
+    to_reverse.apply_modifications()
+    print(os.environ)
+    reversal.apply_modifications()
+    print(os.environ)
+
+    start_env.pop('UNSET')
+    assert os.environ == start_env
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index 63f428dd06..b85ec963e2 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -226,6 +226,16 @@ class UnsetEnv(NameModifier):
         env.pop(self.name, None)
 
 
+class RemoveFlagsEnv(NameValueModifier):
+
+    def execute(self, env):
+        environment_value = env.get(self.name, '')
+        flags = environment_value.split(
+            self.separator) if environment_value else []
+        flags = [f for f in flags if f != self.value]
+        env[self.name] = self.separator.join(flags)
+
+
 class SetPath(NameValueModifier):
 
     def execute(self, env):
@@ -372,6 +382,21 @@ class EnvironmentModifications(object):
         item = UnsetEnv(name, **kwargs)
         self.env_modifications.append(item)
 
+    def remove_flags(self, name, value, sep=' ', **kwargs):
+        """
+        Stores in the current object a request to remove flags from an
+        env variable
+
+        Args:
+            name: name of the environment variable to be removed from
+            value: value to remove to the environment variable
+            sep: separator to assume for environment variable
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        kwargs.update({'separator': sep})
+        item = RemoveFlagsEnv(name, value, **kwargs)
+        self.env_modifications.append(item)
+
     def set_path(self, name, elements, **kwargs):
         """Stores a request to set a path generated from a list.
 
@@ -467,6 +492,40 @@ class EnvironmentModifications(object):
         """
         self.env_modifications = []
 
+    def reversed(self):
+        """
+        Returns the EnvironmentModifications object that will reverse self
+
+        Only creates reversals for additions to the environment, as reversing
+        ``unset`` and ``remove_path`` modifications is impossible.
+
+        Reversable operations are set(), prepend_path(), append_path(),
+        set_path(), and append_flags().
+        """
+        rev = EnvironmentModifications()
+
+        for envmod in reversed(self.env_modifications):
+            if type(envmod) == SetEnv:
+                tty.warn("Reversing `Set` environment operation may lose "
+                         "original value")
+                rev.unset(envmod.name)
+            elif type(envmod) == AppendPath:
+                rev.remove_path(envmod.name, envmod.value)
+            elif type(envmod) == PrependPath:
+                rev.remove_path(envmod.name, envmod.value)
+            elif type(envmod) == SetPath:
+                tty.warn("Reversing `SetPath` environment operation may lose "
+                         "original value")
+                rev.unset(envmod.name)
+            elif type(envmod) == AppendFlagsEnv:
+                rev.remove_flags(envmod.name, envmod.value)
+            else:
+                # This is an un-reversable operation
+                tty.warn("Skipping reversal of unreversable operation"
+                         "%s %s" % (type(envmod), envmod.name))
+
+        return rev
+
     def apply_modifications(self):
         """Applies the modifications and clears the list."""
         modifications = self.group_by_name()
@@ -485,7 +544,8 @@ class EnvironmentModifications(object):
                 x.execute(new_env)
 
         cmds = ''
-        for name in set(new_env) & set(os.environ):
+
+        for name in set(new_env) | set(os.environ):
             new = new_env.get(name, None)
             old = os.environ.get(name, None)
             if new != old:
diff --git a/var/spack/repos/builtin.mock/packages/cmake-client/package.py b/var/spack/repos/builtin.mock/packages/cmake-client/package.py
index e5cadb5d61..40e1c9f9ed 100644
--- a/var/spack/repos/builtin.mock/packages/cmake-client/package.py
+++ b/var/spack/repos/builtin.mock/packages/cmake-client/package.py
@@ -43,7 +43,7 @@ class CmakeClient(CMakePackage):
     def do_not_execute(self):
         self.did_something = True
 
-    def setup_environment(self, spack_env, run_env):
+    def setup_build_environment(self, spack_env):
         spack_cc    # Ensure spack module-scope variable is avaiabl
         check(from_cmake == "from_cmake",
               "setup_environment couldn't read global set by cmake.")
@@ -52,7 +52,7 @@ class CmakeClient(CMakePackage):
               "link arg on dependency spec not readable from "
               "setup_environment.")
 
-    def setup_dependent_environment(self, spack_env, run_env, dspec):
+    def setup_dependent_build_environment(self, spack_env, dspec):
         spack_cc    # Ensure spack module-scope variable is avaiable
         check(from_cmake == "from_cmake",
               "setup_dependent_environment couldn't read global set by cmake.")