From 95a48b27ec09b691d4107d48854126c045b03188 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Tue, 22 Oct 2019 23:27:40 -0700 Subject: [PATCH] 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 | 119 +++++++++++++----- lib/spack/spack/test/cmd/env.py | 2 + lib/spack/spack/test/package_sanity.py | 17 +-- lib/spack/spack/test/util/environment.py | 31 +++++ lib/spack/spack/util/environment.py | 62 ++++++++- .../packages/cmake-client/package.py | 4 +- 6 files changed, 190 insertions(+), 45 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.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): - 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', ['']), - ] + 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'] + } - 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 + 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 @@ 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, 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 @@ 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: 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 @@ 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.")