environment-views: fix bug where missing recipe/repo breaks env commands (#17608)

* environment-views: fix bug where missing recipe/repo breaks env commands

When a recipe or a repo has been removed from Spack and an environment
is active, it causes the view activation to crash Spack before any
commands can be executed. Further, the error message it not at all clear
in explaining the issue.

This forces view regeneration to always start from scratch to avoid the
missing package recipes, and defaults add_view=False in main for views activated
by the `spack -e` option.

* add messages to env status and deactivate

Warn users that a view may be corrupt when deactivating an environment
or checking its status while active. Updated message for activate.

* tests for view checking

Co-authored-by: Gregory Becker <becker33@llnl.gov>

5 files changed, 104 insertions, 19 deletions

diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index d3e825f1dd..e3c45cc27b 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -351,6 +351,9 @@ def env_status(args):
                     % (ev.manifest_name, env.path))
         else:
             tty.msg('In environment %s' % env.name)
+
+        # Check if environment views can be safely activated
+        env.check_views()
     else:
         tty.msg('No active environment')
 
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index f7b50c30c9..151e73a0fd 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -175,9 +175,20 @@ def activate(
     # MANPATH, PYTHONPATH, etc. All variables that end in PATH (case-sensitive)
     # become PATH variables.
     #
-    if add_view and default_view_name in env.views:
-        with spack.store.db.read_transaction():
-            cmds += env.add_default_view_to_shell(shell)
+    try:
+        if add_view and default_view_name in env.views:
+            with spack.store.db.read_transaction():
+                cmds += env.add_default_view_to_shell(shell)
+    except (spack.repo.UnknownPackageError,
+            spack.repo.UnknownNamespaceError) as e:
+        tty.error(e)
+        tty.die(
+            'Environment view is broken due to a missing package or repo.\n',
+            '  To activate without views enabled, activate with:\n',
+            '    spack env activate -V {0}\n'.format(env.name),
+            '  To remove it and resolve the issue, '
+            'force concretize with the command:\n',
+            '    spack -e {0} concretize --force'.format(env.name))
 
     return cmds
 
@@ -230,9 +241,15 @@ def deactivate(shell='sh'):
         cmds += '    unset SPACK_OLD_PS1; export SPACK_OLD_PS1;\n'
         cmds += 'fi;\n'
 
-    if default_view_name in _active_environment.views:
-        with spack.store.db.read_transaction():
-            cmds += _active_environment.rm_default_view_from_shell(shell)
+    try:
+        if default_view_name in _active_environment.views:
+            with spack.store.db.read_transaction():
+                cmds += _active_environment.rm_default_view_from_shell(shell)
+    except (spack.repo.UnknownPackageError,
+            spack.repo.UnknownNamespaceError) as e:
+        tty.warn(e)
+        tty.warn('Could not fully deactivate view due to missing package '
+                 'or repo, shell environment may be corrupt.')
 
     tty.debug("Deactivated environmennt '%s'" % _active_environment.name)
     _active_environment = None
@@ -527,20 +544,26 @@ class ViewDescriptor(object):
             installed_specs_for_view = set(
                 s for s in specs_for_view if s in self and s.package.installed)
 
-            view = self.view()
+            # To ensure there are no conflicts with packages being installed
+            # that cannot be resolved or have repos that have been removed
+            # we always regenerate the view from scratch. We must first make
+            # sure the root directory exists for the very first time though.
+            fs.mkdirp(self.root)
+            with fs.replace_directory_transaction(self.root):
+                view = self.view()
 
-            view.clean()
-            specs_in_view = set(view.get_all_specs())
-            tty.msg("Updating view at {0}".format(self.root))
+                view.clean()
+                specs_in_view = set(view.get_all_specs())
+                tty.msg("Updating view at {0}".format(self.root))
 
-            rm_specs = specs_in_view - installed_specs_for_view
-            add_specs = installed_specs_for_view - specs_in_view
+                rm_specs = specs_in_view - installed_specs_for_view
+                add_specs = installed_specs_for_view - specs_in_view
 
-            # pass all_specs in, as it's expensive to read all the
-            # spec.yaml files twice.
-            view.remove_specs(*rm_specs, with_dependents=False,
-                              all_specs=specs_in_view)
-            view.add_specs(*add_specs, with_dependencies=False)
+                # pass all_specs in, as it's expensive to read all the
+                # spec.yaml files twice.
+                view.remove_specs(*rm_specs, with_dependents=False,
+                                  all_specs=specs_in_view)
+                view.add_specs(*add_specs, with_dependencies=False)
 
 
 class Environment(object):
@@ -1111,6 +1134,24 @@ class Environment(object):
         for view in self.views.values():
             view.regenerate(specs, self.roots())
 
+    def check_views(self):
+        """Checks if the environments default view can be activated."""
+        try:
+            # This is effectively a no-op, but it touches all packages in the
+            # default view if they are installed.
+            for view_name, view in self.views.items():
+                for _, spec in self.concretized_specs():
+                    if spec in view and spec.package.installed:
+                        tty.debug(
+                            'Spec %s in view %s' % (spec.name, view_name))
+        except (spack.repo.UnknownPackageError,
+                spack.repo.UnknownNamespaceError) as e:
+            tty.warn(e)
+            tty.warn(
+                'Environment %s includes out of date packages or repos. '
+                'Loading the environment view will require reconcretization.'
+                % self.name)
+
     def _env_modifications_for_default_view(self, reverse=False):
         all_mods = spack.util.environment.EnvironmentModifications()
 
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index 244913d6f5..b15e99f08b 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -706,7 +706,7 @@ def main(argv=None):
     if not args.no_env:
         env = ev.find_environment(args)
         if env:
-            ev.activate(env, args.use_env_repo)
+            ev.activate(env, args.use_env_repo, add_view=False)
 
     # make spack.config aware of any command line configuration scopes
     if args.config_scopes:
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index b1cc9ce8b5..87f7a58667 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -16,7 +16,7 @@ import spack.environment as ev
 
 from spack.cmd.env import _env_create
 from spack.spec import Spec
-from spack.main import SpackCommand
+from spack.main import SpackCommand, SpackCommandError
 from spack.stage import stage_prefix
 
 from spack.util.mock_package import MockPackageMultiRepo
@@ -284,6 +284,45 @@ def test_environment_status(capsys, tmpdir):
                         assert 'in current directory' in env('status')
 
 
+def test_env_status_broken_view(
+    mutable_mock_env_path, mock_archive, mock_fetch, mock_packages,
+    install_mockery
+):
+    with ev.create('test'):
+        install('trivial-install-test-package')
+
+        # switch to a new repo that doesn't include the installed package
+        # test that Spack detects the missing package and warns the user
+        new_repo = MockPackageMultiRepo()
+        with spack.repo.swap(new_repo):
+            output = env('status')
+            assert 'In environment test' in output
+            assert 'Environment test includes out of date' in output
+
+        # Test that the warning goes away when it's fixed
+        output = env('status')
+        assert 'In environment test' in output
+        assert 'Environment test includes out of date' not in output
+
+
+def test_env_activate_broken_view(
+    mutable_mock_env_path, mock_archive, mock_fetch, mock_packages,
+    install_mockery
+):
+    with ev.create('test'):
+        install('trivial-install-test-package')
+
+    # switch to a new repo that doesn't include the installed package
+    # test that Spack detects the missing package and fails gracefully
+    new_repo = MockPackageMultiRepo()
+    with spack.repo.swap(new_repo):
+        with pytest.raises(SpackCommandError):
+            env('activate', '--sh', 'test')
+
+    # test replacing repo fixes it
+    env('activate', '--sh', 'test')
+
+
 def test_to_lockfile_dict():
     e = ev.create('test')
     e.add('mpileaks')
diff --git a/lib/spack/spack/util/mock_package.py b/lib/spack/spack/util/mock_package.py
index 3d8ae30b10..5e12cc1669 100644
--- a/lib/spack/spack/util/mock_package.py
+++ b/lib/spack/spack/util/mock_package.py
@@ -77,6 +77,8 @@ class MockPackageMultiRepo(object):
     def get(self, spec):
         if not isinstance(spec, spack.spec.Spec):
             spec = Spec(spec)
+        if spec.name not in self.spec_to_pkg:
+            raise spack.repo.UnknownPackageError(spec.fullname)
         return self.spec_to_pkg[spec.name]
 
     def get_pkg_class(self, name):