spack uninstall: reduce verbosity with named environments (#34001)

lib/spack/spack/cmd/uninstall.py

@@ -6,6 +6,7 @@
 from __future__ import print_function
 
 import sys
+from typing import Dict, List, Optional
 
 from llnl.util import tty
 from llnl.util.tty.colify import colify
@@ -16,6 +17,7 @@
 import spack.error
 import spack.package_base
 import spack.repo
+import spack.spec
 import spack.store
 import spack.traverse as traverse
 from spack.database import InstallStatuses
@@ -78,15 +80,19 @@ def setup_parser(subparser):
     )
 
 
-def find_matching_specs(env, specs, allow_multiple_matches=False, force=False, origin=None):
+def find_matching_specs(
+    env: Optional[ev.Environment],
+    specs: List[spack.spec.Spec],
+    allow_multiple_matches: bool = False,
+    origin=None,
+) -> List[spack.spec.Spec]:
     """Returns a list of specs matching the not necessarily
        concretized specs given from cli
 
     Args:
-        env (spack.environment.Environment): active environment, or ``None``
+        env: optional active environment
-            if there is not one
+        specs: list of specs to be matched against installed packages
-        specs (list): list of specs to be matched against installed packages
+        allow_multiple_matches: if True multiple matches are admitted
-        allow_multiple_matches (bool): if True multiple matches are admitted
 
     Return:
         list: list of specs
@@ -128,91 +134,52 @@ def find_matching_specs(env, specs, allow_multiple_matches=False, force=False, o
     return specs_from_cli
 
 
-def installed_runtime_dependents(specs, env):
+def installed_dependents(specs: List[spack.spec.Spec]) -> List[spack.spec.Spec]:
-    """Map each spec to a list of its installed dependents.
+    # Note: the combination of arguments (in particular order=breadth
+    # and root=False) ensures dependents and matching_specs are non-overlapping;
+    # In the extreme case of "spack uninstall --all" we get the entire database as
+    # input; in that case we return an empty list.
 
-    Args:
+    def is_installed(spec):
-        specs (list): list of Specs
+        record = spack.store.db.query_local_by_spec_hash(spec.dag_hash())
-        env (spack.environment.Environment or None): the active environment, or None
+        return record and record.installed
 
-    Returns:
+    specs = traverse.traverse_nodes(
-        tuple: two mappings: one from specs to their dependent installs in the
+        specs,
-        active environment, and one from specs to dependent installs outside of
+        root=False,
-        the active environment.
+        order="breadth",
+        cover="nodes",
+        deptype=("link", "run"),
+        direction="parents",
+        key=lambda s: s.dag_hash(),
+    )
 
-        Every installed dependent spec is listed once.
+    return [spec for spec in specs if is_installed(spec)]
-
-        If there is not current active environment, the first mapping will be
-        empty.
-    """
-    active_dpts = {}
-    outside_dpts = {}
-
-    env_hashes = set(env.all_hashes()) if env else set()
-
-    # Ensure we stop traversal at input specs.
-    visited = set(s.dag_hash() for s in specs)
-
-    for spec in specs:
-        for dpt in traverse.traverse_nodes(
-            spec.dependents(deptype=("link", "run")),
-            direction="parents",
-            visited=visited,
-            deptype=("link", "run"),
-            root=True,
-            key=lambda s: s.dag_hash(),
-        ):
-            hash = dpt.dag_hash()
-            # Ensure that all the specs we get are installed
-            record = spack.store.db.query_local_by_spec_hash(hash)
-            if record is None or not record.installed:
-                continue
-            if hash in env_hashes:
-                active_dpts.setdefault(spec, set()).add(dpt)
-            else:
-                outside_dpts.setdefault(spec, set()).add(dpt)
-
-    return active_dpts, outside_dpts
 
 
-def dependent_environments(specs):
+def dependent_environments(
-    """Map each spec to environments that depend on it.
+    specs: List[spack.spec.Spec], current_env: Optional[ev.Environment] = None
+) -> Dict[ev.Environment, List[spack.spec.Spec]]:
+    # For each tracked environment, get the specs we would uninstall from it.
+    # Don't instantiate current environment twice.
+    env_names = ev.all_environment_names()
+    if current_env:
+        env_names = (name for name in env_names if name != current_env.name)
 
-    Args:
+    # Mapping from Environment -> non-zero list of specs contained in it.
-        specs (list): list of Specs
+    other_envs_to_specs: Dict[ev.Environment, List[spack.spec.Spec]] = {}
+    for other_env in (ev.Environment(ev.root(name)) for name in env_names):
+        specs_in_other_env = all_specs_in_env(other_env, specs)
+        if specs_in_other_env:
+            other_envs_to_specs[other_env] = specs_in_other_env
 
-    Returns:
+    return other_envs_to_specs
-        dict: mapping from spec to lists of dependent Environments
-    """
-    dependents = {}
-    for env in ev.all_environments():
-        hashes = set(env.all_hashes())
-        for spec in specs:
-            if spec.dag_hash() in hashes:
-                dependents.setdefault(spec, []).append(env)
-    return dependents
 
 
-def inactive_dependent_environments(spec_envs):
+def all_specs_in_env(env: ev.Environment, specs: List[spack.spec.Spec]) -> List[spack.spec.Spec]:
-    """Strip the active environment from a dependent map.
+    """Given a list of specs, return those that are in the env"""
-
+    hashes = set(env.all_hashes())
-    Take the output of ``dependent_environment()`` and remove the active
+    return [s for s in specs if s.dag_hash() in hashes]
-    environment from all mappings.  Remove any specs in the map that now
-    have no dependent environments.  Return the result.
-
-    Args:
-        spec_envs (dict): mapping from spec to lists of dependent Environments
-
-    Returns:
-        dict: mapping from spec to lists of *inactive* dependent Environments
-    """
-    spec_inactive_envs = {}
-    for spec, de_list in spec_envs.items():
-        inactive = [de for de in de_list if not de.active]
-        if inactive:
-            spec_inactive_envs[spec] = inactive
-
-    return spec_inactive_envs
 
 
 def _remove_from_env(spec, env):
@@ -225,7 +192,7 @@ def _remove_from_env(spec, env):
         pass  # ignore non-root specs
 
 
-def do_uninstall(specs, force=False):
+def do_uninstall(specs: List[spack.spec.Spec], force: bool = False):
     # TODO: get rid of the call-sites that use this function,
     # so that we don't have to do a dance of list -> set -> list -> set
     hashes_to_remove = set(s.dag_hash() for s in specs)
@@ -237,102 +204,56 @@ def do_uninstall(specs, force=False):
             spack.package_base.PackageBase.uninstall_by_spec(s, force=force)
 
 
-def get_uninstall_list(args, specs, env):
+def get_uninstall_list(args, specs: List[spack.spec.Spec], env: Optional[ev.Environment]):
-    """Returns uninstall_list and remove_list: these may overlap (some things
+    """Returns unordered uninstall_list and remove_list: these may overlap (some things
     may be both uninstalled and removed from the current environment).
 
     It is assumed we are in an environment if --remove is specified (this
-    method raises an exception otherwise).
+    method raises an exception otherwise)."""
-
-    uninstall_list is topologically sorted: dependents come before
-    dependencies (so if a user uninstalls specs in the order provided,
-    the dependents will always be uninstalled first).
-    """
     if args.remove and not env:
         raise ValueError("Can only use --remove when in an environment")
 
     # Gets the list of installed specs that match the ones given via cli
     # args.all takes care of the case where '-a' is given in the cli
-    base_uninstall_specs = set(find_matching_specs(env, specs, args.all, args.force))
+    matching_specs = find_matching_specs(env, specs, args.all)
+    dependent_specs = installed_dependents(matching_specs)
+    all_uninstall_specs = matching_specs + dependent_specs if args.dependents else matching_specs
+    other_dependent_envs = dependent_environments(all_uninstall_specs, current_env=env)
 
-    active_dpts, outside_dpts = installed_runtime_dependents(base_uninstall_specs, env)
+    # There are dependents and we didn't ask to remove dependents
-    # It will be useful to track the unified set of specs with dependents, as
+    dangling_dependents = dependent_specs and not args.dependents
-    # well as to separately track specs in the current env with dependents
-    spec_to_dpts = {}
-    for spec, dpts in active_dpts.items():
-        spec_to_dpts[spec] = list(dpts)
-    for spec, dpts in outside_dpts.items():
-        if spec in spec_to_dpts:
-            spec_to_dpts[spec].extend(dpts)
-        else:
-            spec_to_dpts[spec] = list(dpts)
 
-    all_uninstall_specs = set(base_uninstall_specs)
+    # An environment different than the current env depends on
-    if args.dependents:
+    # one or more of the list of all specs to be uninstalled.
-        for spec, lst in active_dpts.items():
+    dangling_environments = not args.remove and other_dependent_envs
-            all_uninstall_specs.update(lst)
-        for spec, lst in outside_dpts.items():
-            all_uninstall_specs.update(lst)
 
-    # For each spec that we intend to uninstall, this tracks the set of
+    has_error = not args.force and (dangling_dependents or dangling_environments)
-    # environments outside the current active environment which depend on the
-    # spec. There may be environments not managed directly with Spack: such
-    # environments would not be included here.
-    spec_to_other_envs = inactive_dependent_environments(
-        dependent_environments(all_uninstall_specs)
-    )
-
-    has_error = not args.force and (
-        # There are dependents in the current env and we didn't ask to remove
-        # dependents
-        (spec_to_dpts and not args.dependents)
-        # An environment different than the current env (if any) depends on
-        # one or more of the specs to be uninstalled. There may also be
-        # packages in those envs which depend on the base set of packages
-        # to uninstall, but this covers that scenario.
-        or (not args.remove and spec_to_other_envs)
-    )
 
     if has_error:
-        # say why each problem spec is needed
-        specs = set(spec_to_dpts)
-        specs.update(set(spec_to_other_envs))  # environments depend on this
-
-        for i, spec in enumerate(sorted(specs)):
-            # space out blocks of reasons
-            if i > 0:
-                print()
-
-            spec_format = "{name}{@version}{%compiler}{/hash:7}"
-            tty.info("Will not uninstall %s" % spec.cformat(spec_format), format="*r")
-
-            dependents = spec_to_dpts.get(spec)
-            if dependents and not args.dependents:
-                print("The following packages depend on it:")
-                spack.cmd.display_specs(dependents, **display_args)
-
-            envs = spec_to_other_envs.get(spec)
-            if envs:
-                if env:
-                    env_context_qualifier = " other"
-                else:
-                    env_context_qualifier = ""
-                print("It is used by the following{0} environments:".format(env_context_qualifier))
-                colify([e.name for e in envs], indent=4)
-
         msgs = []
-        if spec_to_dpts and not args.dependents:
+        tty.info("Refusing to uninstall the following specs")
+        spack.cmd.display_specs(matching_specs, **display_args)
+        if dangling_dependents:
+            print()
+            tty.info("The following dependents are still installed:")
+            spack.cmd.display_specs(dependent_specs, **display_args)
             msgs.append("use `spack uninstall --dependents` to remove dependents too")
-        if spec_to_other_envs:
+        if dangling_environments:
-            msgs.append("use `spack env remove` to remove from environments")
+            print()
+            tty.info("The following environments still reference these specs:")
+            colify([e.name for e in other_dependent_envs.keys()], indent=4)
+            msgs.append("use `spack env remove` to remove environments")
+        msgs.append("use `spack uninstall --force` to override")
         print()
         tty.die("There are still dependents.", *msgs)
 
     # If we are in an environment, this will track specs in this environment
     # which should only be removed from the environment rather than uninstalled
-    remove_only = set()
+    remove_only = []
     if args.remove and not args.force:
-        remove_only.update(spec_to_other_envs)
+        for specs_in_other_env in other_dependent_envs.values():
+            remove_only.extend(specs_in_other_env)
+
     if remove_only:
         tty.info(
             "The following specs will be removed but not uninstalled because"
@@ -344,23 +265,9 @@ def get_uninstall_list(args, specs, env):
     # Compute the set of specs that should be removed from the current env.
     # This may overlap (some specs may be uninstalled and also removed from
     # the current environment).
-    if args.remove:
+    remove_specs = all_specs_in_env(env, all_uninstall_specs) if env and args.remove else []
-        remove_specs = set(base_uninstall_specs)
-        if args.dependents:
-            # Any spec matched from the cli, or dependent of, should be removed
-            # from the environment
-            for spec, lst in active_dpts.items():
-                remove_specs.update(lst)
-    else:
-        remove_specs = set()
 
-    all_uninstall_specs -= remove_only
+    return list(set(all_uninstall_specs) - set(remove_only)), remove_specs
-    # Inefficient topological sort: uninstall dependents before dependencies
-    all_uninstall_specs = sorted(
-        all_uninstall_specs, key=lambda x: sum(1 for i in x.traverse()), reverse=True
-    )
-
-    return list(all_uninstall_specs), list(remove_specs)
 
 
 def uninstall_specs(args, specs):
@@ -387,13 +294,13 @@ def uninstall_specs(args, specs):
         env.regenerate_views()
 
 
-def confirm_removal(specs):
+def confirm_removal(specs: List[spack.spec.Spec]):
     """Display the list of specs to be removed and ask for confirmation.
 
     Args:
-        specs (list): specs to be removed
+        specs: specs to be removed
     """
-    tty.msg("The following packages will be uninstalled:\n")
+    tty.msg("The following {} packages will be uninstalled:\n".format(len(specs)))
     spack.cmd.display_specs(specs, **display_args)
     print("")
     answer = tty.get_yes_or_no("Do you want to proceed?", default=False)

lib/spack/spack/environment/environment.py

@@ -229,7 +229,7 @@ def deactivate():
     _active_environment = None
 
 
-def active_environment():
+def active_environment() -> Optional["Environment"]:
     """Returns the active environment when there is any"""
     return _active_environment
 

lib/spack/spack/parser.py

@@ -402,8 +402,9 @@ def parse(self, initial_spec: spack.spec.Spec) -> spack.spec.Spec:
             elif not self.has_hash and self.ctx.accept(TokenType.DAG_HASH):
                 dag_hash = self.ctx.current_token.value[1:]
                 matches = []
-                if spack.environment.active_environment():
+                active_env = spack.environment.active_environment()
-                    matches = spack.environment.active_environment().get_by_hash(dag_hash)
+                if active_env:
+                    matches = active_env.get_by_hash(dag_hash)
                 if not matches:
                     matches = spack.store.db.get_by_hash(dag_hash)
                 if not matches:

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

@@ -1049,7 +1049,7 @@ def test_env_blocks_uninstall(mock_stage, mock_fetch, install_mockery):
 
     out = uninstall("mpileaks", fail_on_error=False)
     assert uninstall.returncode == 1
-    assert "used by the following environments" in out
+    assert "The following environments still reference these specs" in out
 
 
 def test_roots_display_with_variants():

lib/spack/spack/test/cmd/uninstall.py

@@ -3,7 +3,6 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import itertools
 import sys
 
 import pytest
@@ -58,14 +57,11 @@ def test_correct_installed_dependents(mutable_database):
     callpath.package.do_uninstall(force=True)
 
     # Retrieve all dependent hashes
-    inside_dpts, outside_dpts = spack.cmd.uninstall.installed_runtime_dependents(
+    dependents = spack.cmd.uninstall.installed_dependents(dependencies)
-        dependencies, None
+    assert dependents
-    )
-    dependent_hashes = [s.dag_hash() for s in itertools.chain(*outside_dpts.values())]
-    set_dependent_hashes = set(dependent_hashes)
 
-    # We dont have an env, so this should be empty.
+    dependent_hashes = [s.dag_hash() for s in dependents]
-    assert not inside_dpts
+    set_dependent_hashes = set(dependent_hashes)
 
     # Assert uniqueness
     assert len(dependent_hashes) == len(set_dependent_hashes)