bugfix: spack shouldn't fail in an incomplete environment (#16473)

Fixed #15884.

Spack asks every package linked into an environment to tell us how
environment variables should be modified when a spack environment is
activated. As part of this, specs in an environment are symlinked into
the environment's view (see #13249), and the package calculates
environment modifications with *the default view as the prefix*.

All of this works nicely for pointing the user's environment at the view
*if* every package is successfully linked. Unfortunately, right now we
only track what specs "should" be in a view, not which specs actually
are. So we end up calculating environment modifications on things that
aren't linked into thee view, and the exception isn't caught, so lots of
spack commands end up failing.

This fixes the issue by ignoring and warning about specs where
calculating environment modifications fails. So we can still keep using
Spack even if the current environment is incomplete.

We should probably also just avoid computing env modifications *entirely*
for unlinked packages, but right now that is a slow operation (requires a
lot of YAML parsing). We should revisit that when we have some better
state management for views, but the fix adopted here will still be
necessary, as we want spack commands to be resilient to other types of
bugs in `setup_run_environment()` and friends. That code is in packages
and we have to assume it could be buggy when we call it outside of builds
(as it might fail more than just the build).

2 files changed, 49 insertions, 9 deletions

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index fff1485e3c..43e91073e3 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -1091,6 +1091,25 @@ class Environment(object):
         for view in self.views.values():
             view.regenerate(specs, self.roots())
 
+    def _env_modifications_for_default_view(self, reverse=False):
+        all_mods = spack.util.environment.EnvironmentModifications()
+
+        errors = []
+        for _, spec in self.concretized_specs():
+            if spec in self.default_view and spec.package.installed:
+                try:
+                    mods = uenv.environment_modifications_for_spec(
+                        spec, self.default_view)
+                except Exception as e:
+                    msg = ("couldn't get environment settings for %s"
+                           % spec.format("{name}@{version} /{hash:7}"))
+                    errors.append((msg, str(e)))
+                    continue
+
+                all_mods.extend(mods.reversed() if reverse else mods)
+
+        return all_mods, errors
+
     def add_default_view_to_shell(self, shell):
         env_mod = spack.util.environment.EnvironmentModifications()
 
@@ -1101,10 +1120,11 @@ class Environment(object):
         env_mod.extend(uenv.unconditional_environment_modifications(
             self.default_view))
 
-        for _, spec in self.concretized_specs():
-            if spec in self.default_view and spec.package.installed:
-                env_mod.extend(uenv.environment_modifications_for_spec(
-                    spec, self.default_view))
+        mods, errors = self._env_modifications_for_default_view()
+        env_mod.extend(mods)
+        if errors:
+            for err in errors:
+                tty.warn(*err)
 
         # deduplicate paths from specs mapped to the same location
         for env_var in env_mod.group_by_name():
@@ -1122,11 +1142,9 @@ class Environment(object):
         env_mod.extend(uenv.unconditional_environment_modifications(
             self.default_view).reversed())
 
-        for _, spec in self.concretized_specs():
-            if spec in self.default_view and spec.package.installed:
-                env_mod.extend(
-                    uenv.environment_modifications_for_spec(
-                        spec, self.default_view).reversed())
+        mods, _ = self._env_modifications_for_default_view(reverse=True)
+        env_mod.extend(mods)
+
         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 671d9869ed..f8ed37e17c 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -163,6 +163,28 @@ def test_env_install_single_spec(install_mockery, mock_fetch):
     assert e.specs_by_hash[e.concretized_order[0]].name == 'cmake-client'
 
 
+def test_env_modifications_error_on_activate(
+        install_mockery, mock_fetch, monkeypatch, capfd):
+    env('create', 'test')
+    install = SpackCommand('install')
+
+    e = ev.read('test')
+    with e:
+        install('cmake-client')
+
+    def setup_error(pkg, env):
+        raise RuntimeError("cmake-client had issues!")
+
+    pkg = spack.repo.path.get_pkg_class("cmake-client")
+    monkeypatch.setattr(pkg, "setup_run_environment", setup_error)
+    with e:
+        pass
+
+    _, err = capfd.readouterr()
+    assert "cmake-client had issues!" in err
+    assert "Warning: couldn't get environment settings" in err
+
+
 def test_env_install_same_spec_twice(install_mockery, mock_fetch, capfd):
     env('create', 'test')