From 35e1dc8eba0ceb8ca9f0b4481343e38cbcdb7cd3 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 5 May 2023 10:23:08 +0200 Subject: spack uninstall: reduce verbosity with named environments (#34001) --- lib/spack/spack/cmd/uninstall.py | 267 ++++++++++------------------- lib/spack/spack/environment/environment.py | 2 +- lib/spack/spack/parser.py | 5 +- lib/spack/spack/test/cmd/env.py | 2 +- lib/spack/spack/test/cmd/uninstall.py | 12 +- 5 files changed, 96 insertions(+), 192 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py index 888ba15f56..9b7a1742ff 100644 --- a/lib/spack/spack/cmd/uninstall.py +++ b/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.environment as ev 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`` - if there is not one - specs (list): list of specs to be matched against installed packages - allow_multiple_matches (bool): if True multiple matches are admitted + env: optional active environment + specs: list of specs to be matched against installed packages + allow_multiple_matches: 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): - """Map each spec to a list of its installed dependents. - - Args: - specs (list): list of Specs - env (spack.environment.Environment or None): the active environment, or None - - Returns: - tuple: two mappings: one from specs to their dependent installs in the - active environment, and one from specs to dependent installs outside of - the active environment. - - Every installed dependent spec is listed once. - - 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): - """Map each spec to environments that depend on it. - - Args: - specs (list): list of Specs +def installed_dependents(specs: List[spack.spec.Spec]) -> List[spack.spec.Spec]: + # 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. + + def is_installed(spec): + record = spack.store.db.query_local_by_spec_hash(spec.dag_hash()) + return record and record.installed + + specs = traverse.traverse_nodes( + specs, + root=False, + order="breadth", + cover="nodes", + deptype=("link", "run"), + direction="parents", + key=lambda s: s.dag_hash(), + ) - Returns: - 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 + return [spec for spec in specs if is_installed(spec)] -def inactive_dependent_environments(spec_envs): - """Strip the active environment from a dependent map. +def dependent_environments( + 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) - Take the output of ``dependent_environment()`` and remove the active - environment from all mappings. Remove any specs in the map that now - have no dependent environments. Return the result. + # Mapping from Environment -> non-zero list of specs contained in it. + 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 - Args: - spec_envs (dict): mapping from spec to lists of dependent Environments + return other_envs_to_specs - 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 all_specs_in_env(env: ev.Environment, specs: List[spack.spec.Spec]) -> List[spack.spec.Spec]: + """Given a list of specs, return those that are in the env""" + hashes = set(env.all_hashes()) + return [s for s in specs if s.dag_hash() in hashes] 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): - """Returns uninstall_list and remove_list: these may overlap (some things +def get_uninstall_list(args, specs: List[spack.spec.Spec], env: Optional[ev.Environment]): + """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). - - 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). - """ + method raises an exception otherwise).""" 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)) - - active_dpts, outside_dpts = installed_runtime_dependents(base_uninstall_specs, env) - # It will be useful to track the unified set of specs with dependents, as - # 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) - if args.dependents: - for spec, lst in active_dpts.items(): - 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 - # 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) - ) + 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) - 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) - ) + # There are dependents and we didn't ask to remove dependents + dangling_dependents = dependent_specs and not args.dependents - 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) + # An environment different than the current env depends on + # one or more of the list of all specs to be uninstalled. + dangling_environments = not args.remove and other_dependent_envs + + has_error = not args.force and (dangling_dependents or dangling_environments) + if has_error: 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: - msgs.append("use `spack env remove` to remove from environments") + if dangling_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 = 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 - # 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 - ) + remove_specs = all_specs_in_env(env, all_uninstall_specs) if env and args.remove else [] - return list(all_uninstall_specs), list(remove_specs) + return list(set(all_uninstall_specs) - set(remove_only)), 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) diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 56b4775b9f..07ed5a868d 100644 --- a/lib/spack/spack/environment/environment.py +++ b/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 diff --git a/lib/spack/spack/parser.py b/lib/spack/spack/parser.py index b4748b259f..c721cdde98 100644 --- a/lib/spack/spack/parser.py +++ b/lib/spack/spack/parser.py @@ -402,8 +402,9 @@ class SpecNodeParser: 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(): - matches = spack.environment.active_environment().get_by_hash(dag_hash) + active_env = spack.environment.active_environment() + 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: diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 3dee4fc389..0fd5a34dd5 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/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(): diff --git a/lib/spack/spack/test/cmd/uninstall.py b/lib/spack/spack/test/cmd/uninstall.py index 50bdf63d46..eea6db1afc 100644 --- a/lib/spack/spack/test/cmd/uninstall.py +++ b/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( - dependencies, None - ) - dependent_hashes = [s.dag_hash() for s in itertools.chain(*outside_dpts.values())] - set_dependent_hashes = set(dependent_hashes) + dependents = spack.cmd.uninstall.installed_dependents(dependencies) + assert dependents - # We dont have an env, so this should be empty. - assert not inside_dpts + dependent_hashes = [s.dag_hash() for s in dependents] + set_dependent_hashes = set(dependent_hashes) # Assert uniqueness assert len(dependent_hashes) == len(set_dependent_hashes) -- cgit v1.2.3-60-g2f50