From 1a3415619ed8e250cf473efdfda1f0e2119d9c3d Mon Sep 17 00:00:00 2001
From: Peter Scheibel <scheibel1@llnl.gov>
Date: Mon, 7 Nov 2022 19:24:51 -0800
Subject: "spack uninstall": don't modify env (#33711)

"spack install foo" no longer adds package "foo" to the environment
(i.e. to the list of root specs) by default: you must specify "--add".
Likewise "spack uninstall foo" no longer removes package "foo" from
the environment: you must specify --remove. Generally this means
that install/uninstall commands will no longer modify the users list
of root specs (which many users found problematic: they had to
deactivate an environment if they wanted to uninstall a spec without
changing their spack.yaml description).

In more detail: if you have environments e1 and e2, and specs [P, Q, R]
such that P depends on R, Q depends on R, [P, R] are in e1, and [Q, R]
are in e2:

* `spack uninstall --dependents --remove r` in e1: removes R from e1
  (but does not uninstall it) and uninstalls (and removes) P
* `spack uninstall -f --dependents r` in e1: will uninstall P, Q, and
   R (i.e. e2 will have dependent specs uninstalled as a side effect)
* `spack uninstall -f --dependents --remove r` in e1: this uninstalls
   P, Q, and R, and removes [P, R] from e1
* `spack uninstall -f --remove r` in e1: uninstalls R (so it is
  "missing" in both environments) and removes R from e1 (note that e1
  would still install R as a dependency of P, but it would no longer
  be listed as a root spec)
* `spack uninstall --dependents r` in e1: will fail because e2 needs R

Individual unit tests were created for each of these scenarios.
---
 lib/spack/spack/cmd/ci.py             |   1 -
 lib/spack/spack/cmd/install.py        |  37 ++++---
 lib/spack/spack/cmd/uninstall.py      | 182 ++++++++++++++++++++++-----------
 lib/spack/spack/test/cmd/ci.py        |   5 +-
 lib/spack/spack/test/cmd/env.py       |  46 ++++++---
 lib/spack/spack/test/cmd/find.py      |   2 +-
 lib/spack/spack/test/cmd/install.py   |  38 +++----
 lib/spack/spack/test/cmd/uninstall.py | 187 ++++++++++++++++++++++++++++++++++
 lib/spack/spack/test/modules/lmod.py  |   2 +-
 9 files changed, 393 insertions(+), 107 deletions(-)

(limited to 'lib')

diff --git a/lib/spack/spack/cmd/ci.py b/lib/spack/spack/cmd/ci.py
index 11a47d5016..6f29da1f4d 100644
--- a/lib/spack/spack/cmd/ci.py
+++ b/lib/spack/spack/cmd/ci.py
@@ -531,7 +531,6 @@ def ci_rebuild(args):
     slash_hash = "/{}".format(job_spec.dag_hash())
     deps_install_args = install_args
     root_install_args = install_args + [
-        "--no-add",
         "--keep-stage",
         "--only=package",
         "--use-buildcache=package:never,dependencies:only",
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index 640b48a6d0..3f9a948a23 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -193,14 +193,22 @@ which is useful for CI pipeline troubleshooting""",
         default=False,
         help="(with environment) only install already concretized specs",
     )
-    subparser.add_argument(
-        "--no-add",
+
+    updateenv_group = subparser.add_mutually_exclusive_group()
+    updateenv_group.add_argument(
+        "--add",
         action="store_true",
         default=False,
-        help="""(with environment) partially install an environment, limiting
-to concrete specs in the environment matching the arguments.
-Non-roots remain installed implicitly.""",
+        help="""(with environment) add spec to the environment as a root.""",
     )
+    updateenv_group.add_argument(
+        "--no-add",
+        action="store_false",
+        dest="add",
+        help="""(with environment) do not add spec to the environment as a
+root (the default behavior).""",
+    )
+
     subparser.add_argument(
         "-f",
         "--file",
@@ -289,11 +297,12 @@ def install_specs_inside_environment(specs, install_kwargs, cli_args):
         # the matches.  Getting to this point means there were either
         # no matches or exactly one match.
 
-        if not m_spec and cli_args.no_add:
+        if not m_spec and not cli_args.add:
             msg = (
-                "You asked to install {0} without adding it (--no-add), but no such spec "
-                "exists in environment"
-            ).format(abstract.name)
+                "Cannot install '{0}' because it is not in the current environment."
+                " You can add it to the environment with 'spack add {0}', or as part"
+                " of the install command with 'spack install --add {0}'"
+            ).format(str(abstract))
             tty.die(msg)
 
         if not m_spec:
@@ -303,14 +312,16 @@ def install_specs_inside_environment(specs, install_kwargs, cli_args):
 
         tty.debug("exactly one match for {0} in env -> {1}".format(m_spec.name, m_spec.dag_hash()))
 
-        if m_spec in env.roots() or cli_args.no_add:
-            # either the single match is a root spec (and --no-add is
-            # the default for roots) or --no-add was stated explicitly
+        if m_spec in env.roots() or not cli_args.add:
+            # either the single match is a root spec (in which case
+            # the spec is not added to the env again), or the user did
+            # not specify --add (in which case it is assumed we are
+            # installing already-concretized specs in the env)
             tty.debug("just install {0}".format(m_spec.name))
             specs_to_install.append(m_spec)
         else:
             # the single match is not a root (i.e. it's a dependency),
-            # and --no-add was not specified, so we'll add it as a
+            # and --add was specified, so we'll add it as a
             # root before installing
             tty.debug("add {0} then install it".format(m_spec.name))
             specs_to_add.append((abstract, concrete))
diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py
index af85b64188..8c112840fd 100644
--- a/lib/spack/spack/cmd/uninstall.py
+++ b/lib/spack/spack/cmd/uninstall.py
@@ -5,7 +5,6 @@
 
 from __future__ import print_function
 
-import itertools
 import sys
 
 from llnl.util import tty
@@ -61,6 +60,13 @@ def setup_parser(subparser):
         dest="force",
         help="remove regardless of whether other packages or environments " "depend on this one",
     )
+    subparser.add_argument(
+        "--remove",
+        action="store_true",
+        dest="remove",
+        help="if in an environment, then the spec should also be removed from "
+        "the environment description",
+    )
     arguments.add_common_arguments(
         subparser, ["recurse_dependents", "yes_to_all", "installed_specs"]
     )
@@ -134,13 +140,21 @@ def installed_dependents(specs, env):
         env (spack.environment.Environment or None): the active environment, or None
 
     Returns:
-        tuple: two mappings: one from specs to their dependent environments in the
-        active environment (or global scope if there is no environment), and one from
-        specs to their dependents in *inactive* environments (empty if there is no
-        environment
+        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.
+
+        Any of the input specs may appear in both mappings (if there are
+        dependents both inside and outside the current environment).
+
+        If a dependent spec is used both by the active environment and by
+        an inactive environment, it will only appear in the first mapping.
+
+        If there is not current active environment, the first mapping will be
+        empty.
     """
     active_dpts = {}
-    inactive_dpts = {}
+    outside_dpts = {}
 
     env_hashes = set(env.all_hashes()) if env else set()
 
@@ -153,12 +167,12 @@ def installed_dependents(specs, env):
         # dpts that are outside this environment
         for dpt in installed:
             if dpt not in specs:
-                if not env or dpt.dag_hash() in env_hashes:
+                if dpt.dag_hash() in env_hashes:
                     active_dpts.setdefault(spec, set()).add(dpt)
                 else:
-                    inactive_dpts.setdefault(spec, set()).add(dpt)
+                    outside_dpts.setdefault(spec, set()).add(dpt)
 
-    return active_dpts, inactive_dpts
+    return active_dpts, outside_dpts
 
 
 def dependent_environments(specs):
@@ -262,31 +276,65 @@ def do_uninstall(env, specs, force):
 
 
 def get_uninstall_list(args, specs, env):
-    # Gets the list of installed specs that match the ones give via cli
-    # args.all takes care of the case where '-a' is given in the cli
-    uninstall_list = find_matching_specs(env, specs, args.all, args.force, args.origin)
+    """Returns 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).
 
-    # Takes care of '-R'
-    active_dpts, inactive_dpts = installed_dependents(uninstall_list, env)
+    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")
 
-    # if we are in the global scope, we complain if you try to remove a
-    # spec that's in an environment.  If we're in an environment, we'll
-    # just *remove* it from the environment, so we ignore this
-    # error when *in* an environment
-    spec_envs = dependent_environments(uninstall_list)
-    spec_envs = inactive_dependent_environments(spec_envs)
+    # 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_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)
+    )
 
-    # Process spec_dependents and update uninstall_list
     has_error = not args.force and (
-        (active_dpts and not args.dependents)  # dependents in the current env
-        or (not env and spec_envs)  # there are environments that need specs
+        # 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)
     )
 
-    # say why each problem spec is needed
     if has_error:
-        specs = set(active_dpts)
-        if not env:
-            specs.update(set(spec_envs))  # environments depend on this
+        # 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
@@ -296,65 +344,85 @@ def get_uninstall_list(args, specs, env):
             spec_format = "{name}{@version}{%compiler}{/hash:7}"
             tty.info("Will not uninstall %s" % spec.cformat(spec_format), format="*r")
 
-            dependents = active_dpts.get(spec)
-            if dependents:
+            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)
 
-            if not env:
-                envs = spec_envs.get(spec)
-                if envs:
-                    print("It is used by the following environments:")
-                    colify([e.name for e in envs], indent=4)
+            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 active_dpts:
+        if spec_to_dpts and not args.dependents:
             msgs.append("use `spack uninstall --dependents` to remove dependents too")
-        if spec_envs:
+        if spec_to_other_envs:
             msgs.append("use `spack env remove` to remove from environments")
         print()
         tty.die("There are still dependents.", *msgs)
 
-    elif args.dependents:
-        for spec, lst in active_dpts.items():
-            uninstall_list.extend(lst)
-        uninstall_list = list(set(uninstall_list))
-
-    # only force-remove (don't completely uninstall) specs that still
-    # have external dependent envs or pkgs
-    removes = set(inactive_dpts)
-    if env:
-        removes.update(spec_envs)
+    # 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()
+    if args.remove and not args.force:
+        remove_only.update(spec_to_other_envs)
+    if remove_only:
+        tty.info(
+            "The following specs will be removed but not uninstalled because"
+            " they are also used by another environment: {speclist}".format(
+                speclist=", ".join(x.name for x in remove_only)
+            )
+        )
 
-    # remove anything in removes from the uninstall list
-    uninstall_list = set(uninstall_list) - removes
+    # 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
+    )
 
-    return uninstall_list, removes
+    return list(all_uninstall_specs), list(remove_specs)
 
 
 def uninstall_specs(args, specs):
     env = ev.active_environment()
 
     uninstall_list, remove_list = get_uninstall_list(args, specs, env)
-    anything_to_do = set(uninstall_list).union(set(remove_list))
 
-    if not anything_to_do:
+    if not uninstall_list:
         tty.warn("There are no package to uninstall.")
         return
 
     if not args.yes_to_all:
-        confirm_removal(anything_to_do)
+        confirm_removal(uninstall_list)
+
+    # Uninstall everything on the list
+    do_uninstall(env, uninstall_list, args.force)
 
     if env:
-        # Remove all the specs that are supposed to be uninstalled or just
-        # removed.
         with env.write_transaction():
-            for spec in itertools.chain(remove_list, uninstall_list):
+            for spec in remove_list:
                 _remove_from_env(spec, env)
             env.write()
 
-    # Uninstall everything on the list
-    do_uninstall(env, uninstall_list, args.force)
+        env.regenerate_views()
 
 
 def confirm_removal(specs):
diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py
index b2e84e0c02..73b474c7df 100644
--- a/lib/spack/spack/test/cmd/ci.py
+++ b/lib/spack/spack/test/cmd/ci.py
@@ -1011,7 +1011,6 @@ def test_ci_rebuild(
 
         assert "--keep-stage" in install_parts
         assert "--no-check-signature" not in install_parts
-        assert "--no-add" in install_parts
         assert "-f" in install_parts
         flag_index = install_parts.index("-f")
         assert "archive-files.json" in install_parts[flag_index + 1]
@@ -1261,7 +1260,7 @@ spack:
             with open(json_path, "w") as ypfd:
                 ypfd.write(spec_json)
 
-            install_cmd("--keep-stage", json_path)
+            install_cmd("--add", "--keep-stage", json_path)
 
             # env, spec, json_path, mirror_url, build_id, sign_binaries
             ci.push_mirror_contents(env, json_path, mirror_url, True)
@@ -1623,7 +1622,7 @@ spack:
             with open(json_path, "w") as ypfd:
                 ypfd.write(spec_json)
 
-            install_cmd("--keep-stage", "-f", json_path)
+            install_cmd("--add", "--keep-stage", "-f", json_path)
             buildcache_cmd("create", "-u", "-a", "-f", "--mirror-url", mirror_url, "callpath")
             ci_cmd("rebuild-index")
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 801ff04669..6782e65179 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -221,7 +221,7 @@ def test_env_install_single_spec(install_mockery, mock_fetch):
 
     e = ev.read("test")
     with e:
-        install("cmake-client")
+        install("--add", "cmake-client")
 
     e = ev.read("test")
     assert e.user_specs[0].name == "cmake-client"
@@ -256,7 +256,7 @@ def test_env_modifications_error_on_activate(install_mockery, mock_fetch, monkey
 
     e = ev.read("test")
     with e:
-        install("cmake-client")
+        install("--add", "cmake-client")
 
     def setup_error(pkg, env):
         raise RuntimeError("cmake-client had issues!")
@@ -277,7 +277,7 @@ def test_activate_adds_transitive_run_deps_to_path(install_mockery, mock_fetch,
 
     e = ev.read("test")
     with e:
-        install("depends-on-run-env")
+        install("--add", "depends-on-run-env")
 
     env_variables = {}
     spack.environment.shell.activate(e).apply_modifications(env_variables)
@@ -290,7 +290,7 @@ def test_env_install_same_spec_twice(install_mockery, mock_fetch):
     e = ev.read("test")
     with e:
         # The first installation outputs the package prefix, updates the view
-        out = install("cmake-client")
+        out = install("--add", "cmake-client")
         assert "Updating view at" in out
 
         # The second installation reports all packages already installed
@@ -449,7 +449,7 @@ def test_env_status_broken_view(
 ):
     env_dir = str(tmpdir)
     with ev.Environment(env_dir):
-        install("trivial-install-test-package")
+        install("--add", "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
@@ -468,7 +468,7 @@ def test_env_activate_broken_view(
     mutable_mock_env_path, mock_archive, mock_fetch, mock_custom_repository, install_mockery
 ):
     with ev.create("test"):
-        install("trivial-install-test-package")
+        install("--add", "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
@@ -1057,7 +1057,9 @@ def test_roots_display_with_variants():
     assert "boost +shared" in out
 
 
-def test_uninstall_removes_from_env(mock_stage, mock_fetch, install_mockery):
+def test_uninstall_keeps_in_env(mock_stage, mock_fetch, install_mockery):
+    # 'spack uninstall' without --remove should not change the environment
+    # spack.yaml file, just uninstall specs
     env("create", "test")
     with ev.read("test"):
         add("mpileaks")
@@ -1065,12 +1067,32 @@ def test_uninstall_removes_from_env(mock_stage, mock_fetch, install_mockery):
         install("--fake")
 
     test = ev.read("test")
-    assert any(s.name == "mpileaks" for s in test.specs_by_hash.values())
-    assert any(s.name == "libelf" for s in test.specs_by_hash.values())
+    # Save this spec to check later if it is still in the env
+    (mpileaks_hash,) = list(x for x, y in test.specs_by_hash.items() if y.name == "mpileaks")
+    orig_user_specs = test.user_specs
+    orig_concretized_specs = test.concretized_order
 
     with ev.read("test"):
         uninstall("-ya")
 
+    test = ev.read("test")
+    assert test.concretized_order == orig_concretized_specs
+    assert test.user_specs.specs == orig_user_specs.specs
+    assert mpileaks_hash in test.specs_by_hash
+    assert not test.specs_by_hash[mpileaks_hash].package.installed
+
+
+def test_uninstall_removes_from_env(mock_stage, mock_fetch, install_mockery):
+    # 'spack uninstall --remove' should update the environment
+    env("create", "test")
+    with ev.read("test"):
+        add("mpileaks")
+        add("libelf")
+        install("--fake")
+
+    with ev.read("test"):
+        uninstall("-y", "-a", "--remove")
+
     test = ev.read("test")
     assert not test.specs_by_hash
     assert not test.concretized_order
@@ -1256,7 +1278,7 @@ def test_env_updates_view_install_package(tmpdir, mock_stage, mock_fetch, instal
     view_dir = tmpdir.join("view")
     env("create", "--with-view=%s" % view_dir, "test")
     with ev.read("test"):
-        install("--fake", "mpileaks")
+        install("--fake", "--add", "mpileaks")
 
     assert os.path.exists(str(view_dir.join(".spack/mpileaks")))
 
@@ -1276,7 +1298,7 @@ def test_env_updates_view_uninstall(tmpdir, mock_stage, mock_fetch, install_mock
     view_dir = tmpdir.join("view")
     env("create", "--with-view=%s" % view_dir, "test")
     with ev.read("test"):
-        install("--fake", "mpileaks")
+        install("--fake", "--add", "mpileaks")
 
     check_mpileaks_and_deps_in_view(view_dir)
 
@@ -1325,7 +1347,7 @@ def test_env_updates_view_force_remove(tmpdir, mock_stage, mock_fetch, install_m
     view_dir = tmpdir.join("view")
     env("create", "--with-view=%s" % view_dir, "test")
     with ev.read("test"):
-        install("--fake", "mpileaks")
+        install("--add", "--fake", "mpileaks")
 
     check_mpileaks_and_deps_in_view(view_dir)
 
diff --git a/lib/spack/spack/test/cmd/find.py b/lib/spack/spack/test/cmd/find.py
index be83541096..9f3f453bad 100644
--- a/lib/spack/spack/test/cmd/find.py
+++ b/lib/spack/spack/test/cmd/find.py
@@ -356,7 +356,7 @@ def test_find_prefix_in_env(
     """Test `find` formats requiring concrete specs work in environments."""
     env("create", "test")
     with ev.read("test"):
-        install("mpileaks")
+        install("--add", "mpileaks")
         find("-p")
         find("-l")
         find("-L")
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 843e539eea..67e0dd3f80 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -771,7 +771,7 @@ def test_install_only_dependencies_in_env(
         dep = Spec("dependency-install").concretized()
         root = Spec("dependent-install").concretized()
 
-        install("-v", "--only", "dependencies", "dependent-install")
+        install("-v", "--only", "dependencies", "--add", "dependent-install")
 
         assert os.path.exists(dep.prefix)
         assert not os.path.exists(root.prefix)
@@ -800,7 +800,7 @@ def test_install_only_dependencies_of_all_in_env(
 
 
 def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock_env_path):
-    # To test behavior of --no-add option, we create the following environment:
+    # To test behavior of --add option, we create the following environment:
     #
     #     mpileaks
     #         ^callpath
@@ -849,18 +849,19 @@ def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock
         # Assert using --no-add with a spec not in the env fails
         inst_out = install("--no-add", "boost", fail_on_error=False, output=str)
 
-        assert "no such spec exists in environment" in inst_out
+        assert "You can add it to the environment with 'spack add " in inst_out
 
-        # Ensure using --no-add with an ambiguous spec fails
+        # Without --add, ensure that install fails if the spec matches more
+        # than one root
         with pytest.raises(ev.SpackEnvironmentError) as err:
-            inst_out = install("--no-add", "a", output=str)
+            inst_out = install("a", output=str)
 
         assert "a matches multiple specs in the env" in str(err)
 
-        # With "--no-add", install an unambiguous dependency spec (that already
-        # exists as a dep in the environment) using --no-add and make sure it
-        # gets installed (w/ deps), but is not added to the environment.
-        install("--no-add", "dyninst")
+        # Install an unambiguous dependency spec (that already exists as a dep
+        # in the environment) and make sure it gets installed (w/ deps),
+        # but is not added to the environment.
+        install("dyninst")
 
         find_output = find("-l", output=str)
         assert "dyninst" in find_output
@@ -872,31 +873,30 @@ def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock
         assert all([s in env_specs for s in post_install_specs])
 
         # Make sure we can install a concrete dependency spec from a spec.json
-        # file on disk, using the ``--no-add` option, and the spec is installed
-        # but not added as a root
+        # file on disk, and the spec is installed but not added as a root
         mpi_spec_json_path = tmpdir.join("{0}.json".format(mpi_spec.name))
         with open(mpi_spec_json_path.strpath, "w") as fd:
             fd.write(mpi_spec.to_json(hash=ht.dag_hash))
 
-        install("--no-add", "-f", mpi_spec_json_path.strpath)
+        install("-f", mpi_spec_json_path.strpath)
         assert mpi_spec not in e.roots()
 
         find_output = find("-l", output=str)
         assert mpi_spec.name in find_output
 
-        # Without "--no-add", install an unambiguous depependency spec (that
-        # already exists as a dep in the environment) without --no-add and make
-        # sure it is added as a root of the environment as well as installed.
+        # Install an unambiguous depependency spec (that already exists as a
+        # dep in the environment) with --add and make sure it is added as a
+        # root of the environment as well as installed.
         assert b_spec not in e.roots()
 
-        install("b")
+        install("--add", "b")
 
         assert b_spec in e.roots()
         assert b_spec not in e.uninstalled_specs()
 
-        # Without "--no-add", install a novel spec and make sure it is added
-        # as a root and installed.
-        install("bowtie")
+        # Install a novel spec with --add and make sure it is added  as a root
+        # and installed.
+        install("--add", "bowtie")
 
         assert any([s.name == "bowtie" for s in e.roots()])
         assert not any([s.name == "bowtie" for s in e.uninstalled_specs()])
diff --git a/lib/spack/spack/test/cmd/uninstall.py b/lib/spack/spack/test/cmd/uninstall.py
index a47f6c60a5..ddbd54e252 100644
--- a/lib/spack/spack/test/cmd/uninstall.py
+++ b/lib/spack/spack/test/cmd/uninstall.py
@@ -3,10 +3,13 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import sys
+
 import pytest
 
 import llnl.util.tty as tty
 
+import spack.environment
 import spack.store
 from spack.main import SpackCommand, SpackCommandError
 
@@ -166,3 +169,187 @@ def test_in_memory_consistency_when_uninstalling(mutable_database, monkeypatch):
     monkeypatch.setattr(tty, "warn", _warn)
     # Now try to uninstall and check this doesn't trigger warnings
     uninstall("-y", "-a")
+
+
+# Note: I want to use https://docs.pytest.org/en/7.1.x/how-to/skipping.html#skip-all-test-functions-of-a-class-or-module
+# the style formatter insists on separating these two lines.
+pytest.mark.skipif(sys.platform == "win32", reason="Envs unsupported on Windows")
+
+
+class TestUninstallFromEnv(object):
+    """Tests an installation with two environments e1 and e2, which each have
+    shared package installations:
+
+    e1 has dt-diamond-left -> dt-diamond-bottom
+
+    e2 has dt-diamond-right -> dt-diamond-bottom
+    """
+
+    env = SpackCommand("env")
+    add = SpackCommand("add")
+    concretize = SpackCommand("concretize")
+    find = SpackCommand("find")
+
+    @pytest.fixture
+    def environment_setup(
+        self, mutable_mock_env_path, config, mock_packages, mutable_database, install_mockery
+    ):
+        TestUninstallFromEnv.env("create", "e1")
+        e1 = spack.environment.read("e1")
+        with e1:
+            TestUninstallFromEnv.add("dt-diamond-left")
+            TestUninstallFromEnv.add("dt-diamond-bottom")
+            TestUninstallFromEnv.concretize()
+            install("--fake")
+
+        TestUninstallFromEnv.env("create", "e2")
+        e2 = spack.environment.read("e2")
+        with e2:
+            TestUninstallFromEnv.add("dt-diamond-right")
+            TestUninstallFromEnv.add("dt-diamond-bottom")
+            TestUninstallFromEnv.concretize()
+            install("--fake")
+
+    def test_basic_env_sanity(self, environment_setup):
+        for env_name in ["e1", "e2"]:
+            e = spack.environment.read(env_name)
+            with e:
+                for _, concretized_spec in e.concretized_specs():
+                    assert concretized_spec.package.installed
+
+    def test_uninstall_force_dependency_shared_between_envs(self, environment_setup):
+        """If you "spack uninstall -f --dependents dt-diamond-bottom" from
+        e1, then all packages should be uninstalled (but not removed) from
+        both e1 and e2.
+        """
+        e1 = spack.environment.read("e1")
+        with e1:
+            uninstall("-f", "-y", "--dependents", "dt-diamond-bottom")
+
+            # The specs should still be in the environment, since
+            # --remove was not specified
+            assert set(root.name for (root, _) in e1.concretized_specs()) == set(
+                ["dt-diamond-left", "dt-diamond-bottom"]
+            )
+
+            for _, concretized_spec in e1.concretized_specs():
+                assert not concretized_spec.package.installed
+
+        # Everything in e2 depended on dt-diamond-bottom, so should also
+        # have been uninstalled. The roots should be unchanged though.
+        e2 = spack.environment.read("e2")
+        with e2:
+            assert set(root.name for (root, _) in e2.concretized_specs()) == set(
+                ["dt-diamond-right", "dt-diamond-bottom"]
+            )
+            for _, concretized_spec in e2.concretized_specs():
+                assert not concretized_spec.package.installed
+
+    def test_uninstall_remove_dependency_shared_between_envs(self, environment_setup):
+        """If you "spack uninstall --dependents --remove dt-diamond-bottom" from
+        e1, then all packages are removed from e1 (it is now empty);
+        dt-diamond-left is also uninstalled (since only e1 needs it) but
+        dt-diamond-bottom is not uninstalled (since e2 needs it).
+        """
+        e1 = spack.environment.read("e1")
+        with e1:
+            dtdiamondleft = next(
+                concrete
+                for (_, concrete) in e1.concretized_specs()
+                if concrete.name == "dt-diamond-left"
+            )
+            output = uninstall("-y", "--dependents", "--remove", "dt-diamond-bottom")
+            assert "The following specs will be removed but not uninstalled" in output
+            assert not list(e1.roots())
+            assert not dtdiamondleft.package.installed
+
+        # Since -f was not specified, all specs in e2 should still be installed
+        # (and e2 should be unchanged)
+        e2 = spack.environment.read("e2")
+        with e2:
+            assert set(root.name for (root, _) in e2.concretized_specs()) == set(
+                ["dt-diamond-right", "dt-diamond-bottom"]
+            )
+            for _, concretized_spec in e2.concretized_specs():
+                assert concretized_spec.package.installed
+
+    def test_uninstall_dependency_shared_between_envs_fail(self, environment_setup):
+        """If you "spack uninstall --dependents dt-diamond-bottom" from
+        e1 (without --remove or -f), then this should fail (this is needed by
+        e2).
+        """
+        e1 = spack.environment.read("e1")
+        with e1:
+            output = uninstall("-y", "--dependents", "dt-diamond-bottom", fail_on_error=False)
+            assert "There are still dependents." in output
+            assert "use `spack env remove`" in output
+
+        # The environment should be unchanged and nothing should have been
+        # uninstalled
+        assert set(root.name for (root, _) in e1.concretized_specs()) == set(
+            ["dt-diamond-left", "dt-diamond-bottom"]
+        )
+        for _, concretized_spec in e1.concretized_specs():
+            assert concretized_spec.package.installed
+
+    def test_uninstall_force_and_remove_dependency_shared_between_envs(self, environment_setup):
+        """If you "spack uninstall -f --dependents --remove dt-diamond-bottom" from
+        e1, then all packages should be uninstalled and removed from e1.
+        All packages will also be uninstalled from e2, but the roots will
+        remain unchanged.
+        """
+        e1 = spack.environment.read("e1")
+        with e1:
+            dtdiamondleft = next(
+                concrete
+                for (_, concrete) in e1.concretized_specs()
+                if concrete.name == "dt-diamond-left"
+            )
+            uninstall("-f", "-y", "--dependents", "--remove", "dt-diamond-bottom")
+            assert not list(e1.roots())
+            assert not dtdiamondleft.package.installed
+
+        e2 = spack.environment.read("e2")
+        with e2:
+            assert set(root.name for (root, _) in e2.concretized_specs()) == set(
+                ["dt-diamond-right", "dt-diamond-bottom"]
+            )
+            for _, concretized_spec in e2.concretized_specs():
+                assert not concretized_spec.package.installed
+
+    def test_uninstall_keep_dependents_dependency_shared_between_envs(self, environment_setup):
+        """If you "spack uninstall -f --remove dt-diamond-bottom" from
+        e1, then dt-diamond-bottom should be uninstalled, which leaves
+        "dangling" references in both environments, since
+        dt-diamond-left and dt-diamond-right both need it.
+        """
+        e1 = spack.environment.read("e1")
+        with e1:
+            dtdiamondleft = next(
+                concrete
+                for (_, concrete) in e1.concretized_specs()
+                if concrete.name == "dt-diamond-left"
+            )
+            uninstall("-f", "-y", "--remove", "dt-diamond-bottom")
+            # dt-diamond-bottom was removed from the list of roots (note that
+            # it would still be installed since dt-diamond-left depends on it)
+            assert set(x.name for x in e1.roots()) == set(["dt-diamond-left"])
+            assert dtdiamondleft.package.installed
+
+        e2 = spack.environment.read("e2")
+        with e2:
+            assert set(root.name for (root, _) in e2.concretized_specs()) == set(
+                ["dt-diamond-right", "dt-diamond-bottom"]
+            )
+            dtdiamondright = next(
+                concrete
+                for (_, concrete) in e2.concretized_specs()
+                if concrete.name == "dt-diamond-right"
+            )
+            assert dtdiamondright.package.installed
+            dtdiamondbottom = next(
+                concrete
+                for (_, concrete) in e2.concretized_specs()
+                if concrete.name == "dt-diamond-bottom"
+            )
+            assert not dtdiamondbottom.package.installed
diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py
index d24ec8ca0d..5d0d04032e 100644
--- a/lib/spack/spack/test/modules/lmod.py
+++ b/lib/spack/spack/test/modules/lmod.py
@@ -298,7 +298,7 @@ class TestLmod(object):
     ):
         with ev.Environment(str(tmpdir), with_view=True) as e:
             module_configuration("with_view")
-            install("cmake")
+            install("--add", "cmake")
 
             spec = spack.spec.Spec("cmake").concretized()
 
-- 
cgit v1.2.3-60-g2f50