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.

lib/spack/spack/environment.py

@@ -28,9 +28,11 @@
 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 @@ def view(self):
                                   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 @@ def regenerate(self, all_specs, roots):
             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 @@ def regenerate_views(self):
         for view in self.views.values():
             view.regenerate(specs, self.roots())
 
-    def _shell_vars(self):
+    prefix_inspections = {
-        updates = [
+        'bin': ['PATH'],
-            ('PATH', ['bin']),
+        'lib': ['LD_LIBRARY_PATH', 'LIBRARY_PATH', 'DYLD_LIBRARY_PATH'],
-            ('MANPATH', ['man', 'share/man']),
+        'lib64': ['LD_LIBRARY_PATH', 'LIBRARY_PATH', 'DYLD_LIBRARY_PATH'],
-            ('ACLOCAL_PATH', ['share/aclocal']),
+        'man': ['MANPATH'],
-            ('LD_LIBRARY_PATH', ['lib', 'lib64']),
+        'share/man': ['MANPATH'],
-            ('LIBRARY_PATH', ['lib', 'lib64']),
+        'share/aclocal': ['ACLOCAL_PATH'],
-            ('CPATH', ['include']),
+        'include': ['CPATH'],
-            ('PKG_CONFIG_PATH', ['lib/pkgconfig', 'lib64/pkgconfig',
+        'lib/pkgconfig': ['PKG_CONFIG_PATH'],
-                                 'share/pkgconfig']),
+        'lib64/pkgconfig': ['PKG_CONFIG_PATH'],
-            ('CMAKE_PREFIX_PATH', ['']),
+        '': ['CMAKE_PREFIX_PATH']
-        ]
+    }
 
-        path_updates = list()
+    def environment_modifications_for_spec(self, spec, view=None):
-        if default_view_name in self.views:
+        """List of environment modifications to be processed."""
-            for var, dirs in updates:
+        spec = spec.copy()
-                paths = [os.path.join(self.default_view.root, x) for x in dirs]
+        if view:
-                path_updates.append((var, paths))
+            spec.prefix = Prefix(view.view().get_projection_for_spec(spec))
-        return path_updates
+
+        # 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()
+        env_mod = spack.util.environment.EnvironmentModifications()
-        for var, paths in self._shell_vars():
+
-            for path in paths:
+        if default_view_name not in self.views:
-                env_mod.prepend_path(var, path)
+            # 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()
+        env_mod = spack.util.environment.EnvironmentModifications()
-        for var, paths in self._shell_vars():
+
-            for path in paths:
+        if default_view_name not in self.views:
-                env_mod.remove_path(var, path)
+            # 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):

lib/spack/spack/test/cmd/env.py

@@ -1577,8 +1577,10 @@ def test_stack_view_activate_from_default(tmpdir, mock_fetch, mock_packages,
             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,

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))
+        if 'builtin.mock' not in file:  # don't restrict packages for tests
-        pkg = spack.repo.get(name)
+            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)
     )

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

lib/spack/spack/util/environment.py

@@ -226,6 +226,16 @@ def execute(self, env):
         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 @@ def unset(self, name, **kwargs):
         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 @@ def clear(self):
         """
         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 @@ def shell_modifications(self, shell='sh'):
                 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:

var/spack/repos/builtin.mock/packages/cmake-client/package.py

@@ -43,7 +43,7 @@ def flip(self):
     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 @@ def setup_environment(self, spack_env, run_env):
               "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.")