env: move `env uninstall` into `spack uninstall`

- uninstall now:
  - restricts its spec search to the current environment
  - removes uninstalled specs from the current environment
  - reports envs that still need specs you're trying to uninstall

- removed spack env uninstall command
- updated tests

6 files changed, 191 insertions, 72 deletions

diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 39a5bcd234..e207617009 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -7,6 +7,7 @@ from __future__ import print_function
 
 import os
 import re
+import sys
 
 import llnl.util.tty as tty
 from llnl.util.lang import attr_setdefault, index_by
@@ -207,6 +208,7 @@ def display_specs(specs, args=None, **kwargs):
         namespace (bool): Print namespaces along with names
         show_flags (bool): Show compiler flags with specs
         variants (bool): Show variants with specs
+        indent (int): indent each line this much
 
     """
     def get_arg(name, default=None):
@@ -225,6 +227,9 @@ def display_specs(specs, args=None, **kwargs):
     full_compiler = get_arg('show_full_compiler', False)
     variants  = get_arg('variants', False)
 
+    indent = get_arg('indent', 0)
+    ispace = indent * ' '
+
     hlen = 7
     if get_arg('very_long', False):
         hashes = True
@@ -255,6 +260,7 @@ def display_specs(specs, args=None, **kwargs):
         # If they don't have a compiler / architecture attached to them,
         # then skip the header
         if architecture is not None or compiler is not None:
+            sys.stdout.write(ispace)
             tty.hline(colorize(header), char='-')
 
         specs = index[(architecture, compiler)]
@@ -269,7 +275,7 @@ def display_specs(specs, args=None, **kwargs):
 
             for abbrv, spec in zip(abbreviated, specs):
                 prefix = gray_hash(spec, hlen) if hashes else ''
-                print(prefix + (format % (abbrv, spec.prefix)))
+                print(ispace + prefix + (format % (abbrv, spec.prefix)))
 
         elif mode == 'deps':
             for spec in specs:
@@ -290,13 +296,13 @@ def display_specs(specs, args=None, **kwargs):
 
                     return string
 
-                colify(fmt(s) for s in specs)
+                colify((fmt(s) for s in specs), indent=indent)
             # Print one entry per line if including flags
             else:
                 for spec in specs:
                     # Print the hash if necessary
                     hsh = gray_hash(spec, hlen) + ' ' if hashes else ''
-                    print(hsh + spec.cformat(format_string) + '\n')
+                    print(ispace + hsh + spec.cformat(format_string))
 
         else:
             raise ValueError(
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 6b20fbc014..671dde4fa0 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -56,7 +56,7 @@ class ConstraintAction(argparse.Action):
         # only its installed packages.
         env = spack.environment.active
         if env:
-            kwargs['hashes'] = set(env.specs_by_hash.keys())
+            kwargs['hashes'] = set(env.all_hashes())
 
         # return everything for an empty query.
         if not qspecs:
diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index 776f6f0544..2d5f95f640 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -34,7 +34,6 @@ subcommands = [
     ['list', 'ls'],
     ['status', 'st'],
     'loads',
-    'uninstall',
 ]
 
 
@@ -234,7 +233,7 @@ def env_remove(args):
     for env_name in args.env:
         env = ev.disambiguate(env_name)
 
-        if ev.active and ev.active.path == env.path:
+        if env.active:
             tty.die("Environment %s can't be removed while activated.")
 
         env.destroy()
@@ -250,7 +249,7 @@ def env_list_setup_parser(subparser):
 
 
 def env_list(args):
-    names = ev.list_environments()
+    names = ev.all_environment_names()
 
     color_names = []
     for name in names:
@@ -268,21 +267,6 @@ def env_list(args):
     colify(color_names, indent=4)
 
 
-# REMOVE
-# env uninstall
-#
-def env_uninstall_setup_parser(subparser):
-    """uninstall packages from an environment"""
-    subparser.add_argument(
-        'env', nargs='?', help='uninstall all packages in this environment')
-    spack.cmd.uninstall.add_common_arguments(subparser)
-
-
-def env_uninstall(args):
-    env = ev.get_env(args, 'env uninstall')
-    env.uninstall(args)
-
-
 #
 # env status
 #
@@ -308,6 +292,7 @@ def env_status(args):
         hashlen=None if args.very_long else 7,
         install_status=True)
 
+
 #
 # env loads
 #
diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py
index 8fea7c0ac2..6425e88d9e 100644
--- a/lib/spack/spack/cmd/uninstall.py
+++ b/lib/spack/spack/cmd/uninstall.py
@@ -8,12 +8,14 @@ from __future__ import print_function
 import argparse
 
 import spack.cmd
+import spack.environment as ev
 import spack.package
 import spack.cmd.common.arguments as arguments
 import spack.repo
 import spack.store
 
 from llnl.util import tty
+from llnl.util.tty.colify import colify
 
 description = "remove installed packages"
 section = "build"
@@ -28,14 +30,16 @@ error_message = """You can either:
 display_args = {
     'long': True,
     'show_flags': True,
-    'variants': True
+    'variants': True,
+    'indent': 4,
 }
 
 
 def add_common_arguments(subparser):
     subparser.add_argument(
         '-f', '--force', action='store_true', dest='force',
-        help="remove regardless of whether other packages depend on this one")
+        help="remove regardless of whether other packages or environments "
+        "depend on this one")
     arguments.add_common_arguments(
         subparser, ['recurse_dependents', 'yes_to_all'])
 
@@ -44,10 +48,12 @@ def setup_parser(subparser):
     add_common_arguments(subparser)
     subparser.add_argument(
         '-a', '--all', action='store_true', dest='all',
-        help="USE CAREFULLY. remove ALL installed packages that match each "
-             "supplied spec. i.e., if you `uninstall --all libelf`,"
-             " ALL versions of `libelf` are uninstalled. if no spec is "
-             "supplied all installed software will be uninstalled.")
+        help="USE CAREFULLY. Remove ALL installed packages that match each "
+        "supplied spec. i.e., if you `uninstall --all libelf`,"
+        " ALL versions of `libelf` are uninstalled. If no spec is "
+        "supplied, all installed packages will be uninstalled. "
+        "If used in an environment, all packages in the environment "
+        "will be uninstalled.")
 
     subparser.add_argument(
         'packages',
@@ -66,11 +72,13 @@ def find_matching_specs(specs, allow_multiple_matches=False, force=False):
     Return:
         list of specs
     """
+    hashes = ev.active.all_hashes() if ev.active else None
+
     # List of specs that match expressions given via command line
     specs_from_cli = []
     has_errors = False
     for spec in specs:
-        matching = spack.store.db.query(spec)
+        matching = spack.store.db.query(spec, hashes=hashes)
         # For each spec provided, make sure it refers to only one package.
         # Fail and ask user to be unambiguous if it doesn't
         if not allow_multiple_matches and len(matching) > 1:
@@ -82,11 +90,14 @@ def find_matching_specs(specs, allow_multiple_matches=False, force=False):
 
         # No installed package matches the query
         if len(matching) == 0 and spec is not any:
-            tty.error('{0} does not match any installed packages.'.format(
-                spec))
-            has_errors = True
+            if ev.active:
+                pkg_type = "packages in environment '%s'" % ev.active.name
+            else:
+                pkg_type = 'installed packages'
+            tty.die('{0} does not match any {1}.'.format(spec, pkg_type))
 
         specs_from_cli.extend(matching)
+
     if has_errors:
         tty.die(error_message)
 
@@ -94,14 +105,14 @@ def find_matching_specs(specs, allow_multiple_matches=False, force=False):
 
 
 def installed_dependents(specs):
-    """Returns a dictionary that maps a spec with a list of its
-    installed dependents
+    """Map each spec to a list of its installed dependents.
 
     Args:
-        specs: list of specs to be checked for dependents
+        specs (list): list of Specs
 
     Returns:
-        dictionary of installed dependents
+        (dict): mapping from spec to lists of Environments
+
     """
     dependents = {}
     for item in specs:
@@ -114,6 +125,28 @@ def installed_dependents(specs):
     return dependents
 
 
+def dependent_environments(specs):
+    """Map each spec to environments that depend on it.
+
+    This excludes the active environment, because we allow uninstalling
+    from the active environment.
+
+    Args:
+        specs (list): list of Specs
+    Returns:
+        (dict): mapping from spec to lists of Environments
+
+    """
+    dependents = {}
+    for env in ev.all_environments():
+        if not env.active:
+            hashes = set([s.dag_hash() for s in env.all_specs()])
+            for spec in specs:
+                if spec.dag_hash() in hashes:
+                    dependents.setdefault(spec, []).append(env)
+    return dependents
+
+
 def do_uninstall(specs, force):
     """
     Uninstalls all the specs in a list.
@@ -132,6 +165,12 @@ def do_uninstall(specs, force):
             # want to uninstall.
             spack.package.Package.uninstall_by_spec(item, force=True)
 
+        if ev.active:
+            try:
+                ev.active.remove(item, force=True)
+            except ev.EnvError:
+                pass  # ignore errors from specs that are not roots
+
     # Sort packages to be uninstalled by the number of installed dependents
     # This ensures we do things in the right order
     def num_installed_deps(pkg):
@@ -143,6 +182,10 @@ def do_uninstall(specs, force):
     for item in packages:
         item.do_uninstall(force=force)
 
+    # write any changes made to the active environment
+    if ev.active:
+        ev.active.write()
+
 
 def get_uninstall_list(args, specs):
     # Gets the list of installed specs that match the ones give via cli
@@ -150,25 +193,54 @@ def get_uninstall_list(args, specs):
     uninstall_list = find_matching_specs(specs, args.all, args.force)
 
     # Takes care of '-R'
-    dependent_list = installed_dependents(uninstall_list)
-
-    # Process dependent_list and update uninstall_list
-    has_error = False
-    if dependent_list and not args.dependents and not args.force:
-        for spec, lst in dependent_list.items():
-            tty.error("Will not uninstall %s" % spec.cformat("$_$@$%@$/"))
-            print('')
-            print('The following packages depend on it:')
-            spack.cmd.display_specs(lst, **display_args)
-            print('')
-            has_error = True
+    spec_dependents = installed_dependents(uninstall_list)
+    spec_envs = dependent_environments(uninstall_list)
+
+    # Process spec_dependents and update uninstall_list
+    has_error = not args.force and (
+        (spec_dependents and not args.dependents) or
+        spec_envs)
+
+    # say why each problem spec is needed
+    if has_error:
+        specs = set(list(spec_dependents.keys()) + list(spec_envs.keys()))
+        for i, spec in enumerate(sorted(specs)):
+            # space out blocks of reasons
+            if i > 0:
+                print()
+
+            tty.info("Will not uninstall %s" % spec.cformat("$_$@$%@$/"),
+                     format='*r')
+
+            dependents = spec_dependents.get(spec)
+            if dependents:
+                print('The following packages depend on it:')
+                spack.cmd.display_specs(dependents, **display_args)
+
+            envs = spec_envs.get(spec)
+            if envs:
+                print('It is used by the following environments:')
+                colify([e.name for e in envs], indent=4)
+
+        msgs = []
+        if spec_dependents:
+            msgs.append(
+                'use `spack uninstall --dependents` to uninstall dependents '
+                'as well.')
+        if spec_envs:
+            msgs.append(
+                'use `spack env remove` to remove environments, or '
+                '`spack remove` to remove specs from environments.')
+        if ev.active:
+            msgs.append('consider using `spack remove` to remove the spec '
+                        'from this environment')
+        print()
+        tty.die('There are still dependents.', *msgs)
+
     elif args.dependents:
-        for key, lst in dependent_list.items():
+        for spec, lst in spec_dependents.items():
             uninstall_list.extend(lst)
         uninstall_list = list(set(uninstall_list))
-    if has_error:
-        tty.die('Use `spack uninstall --dependents` '
-                'to uninstall these dependencies as well.')
 
     return uninstall_list
 
@@ -196,5 +268,6 @@ def uninstall(parser, args):
         tty.die('uninstall requires at least one package argument.',
                 '  Use `spack uninstall --all` to uninstall ALL packages.')
 
+    # [any] here handles the --all case by forcing all specs to be returned
     uninstall_specs(
         args, spack.cmd.parse_specs(args.packages) if args.packages else [any])
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 87f4d31026..c221aab855 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -242,7 +242,7 @@ def config_dict(yaml_data):
     return yaml_data[key]
 
 
-def list_environments():
+def all_environment_names():
     """List the names of environments that currently exist."""
     # just return empty if the env path does not exist.  A read-only
     # operation like list should not try to create a directory.
@@ -258,6 +258,12 @@ def list_environments():
     return names
 
 
+def all_environments():
+    """Generator for all named Environments."""
+    for name in all_environment_names():
+        yield read(name)
+
+
 def validate(data, filename=None):
     global _validator
     if _validator is None:
@@ -363,6 +369,11 @@ class Environment(object):
             return self.path
 
     @property
+    def active(self):
+        """True if this environment is currently active."""
+        return active and self.path == active.path
+
+    @property
     def manifest_path(self):
         """Path to spack.yaml file in this environment."""
         return os.path.join(self.path, manifest_name)
@@ -480,8 +491,7 @@ class Environment(object):
             specs_hashes = zip(
                 self.concretized_user_specs, self.concretized_order)
             matches = [
-                s for s, h in specs_hashes
-                if s.satisfies(query_spec) or query_spec.dag_hash() == h]
+                s for s, h in specs_hashes if query_spec.dag_hash() == h]
 
         if not matches:
             raise EnvError("Not found: {0}".format(query_spec))
@@ -615,12 +625,6 @@ class Environment(object):
                     os.remove(build_log_link)
                 os.symlink(spec.package.build_log_path, build_log_link)
 
-    def uninstall(self, args):
-        """Uninstall all the specs in an Environment."""
-        specs = self._get_environment_specs(recurse_dependencies=True)
-        args.all = False
-        spack.cmd.uninstall.uninstall_specs(args, specs)
-
     def status(self, stream, **kwargs):
         """List the specs in an environment."""
         tty.msg('In environment %s' % self.name)
@@ -635,10 +639,10 @@ class Environment(object):
         current = [(s, c) for s, c in concretized if s in self.user_specs]
 
         def write_kind(s):
-            color.cwrite('@c{%s}\n' % str(s), stream)
+            color.cwrite('@c{%s}\n' % color.cescape(s), stream)
 
         def write_user_spec(s, c):
-            color.cwrite('@%s{----} %s\n' % (c, str(s)), stream)
+            color.cwrite('@%s{----} %s\n' % (c, color.cescape(s)), stream)
 
         if added:
             write_kind('added:')
@@ -661,23 +665,44 @@ class Environment(object):
             write_user_spec(s, 'r')
             stream.write(c.tree(**kwargs))
 
+    def all_specs_by_hash(self):
+        """Map of hashes to spec for all specs in this environment."""
+        hashes = {}
+        for h in self.concretized_order:
+            specs = self.specs_by_hash[h].traverse(deptype=('link', 'run'))
+            for spec in specs:
+                hashes[spec.dag_hash()] = spec
+        return hashes
+
+    def all_specs(self):
+        """Return all specs, even those a user spec would shadow."""
+        return sorted(self.all_specs_by_hash().values())
+
+    def all_hashes(self):
+        """Return all specs, even those a user spec would shadow."""
+        return list(self.all_specs_by_hash().keys())
+
     def _get_environment_specs(self, recurse_dependencies=True):
         """Returns the specs of all the packages in an environment.
+
         If these specs appear under different user_specs, only one copy
-        is added to the list returned."""
+        is added to the list returned.
+        """
         package_to_spec = {}
         spec_list = list()
 
         for spec_hash in self.concretized_order:
             spec = self.specs_by_hash[spec_hash]
 
-            specs = spec.traverse(deptype=('link', 'run')) \
-                if recurse_dependencies else (spec,)
+            specs = (spec.traverse(deptype=('link', 'run'))
+                     if recurse_dependencies else (spec,))
+
             for dep in specs:
-                if dep.name in package_to_spec:
-                    tty.warn("{0} takes priority over {1}"
-                             .format(package_to_spec[dep.name].format(),
-                                     dep.format()))
+                prior = package_to_spec.get(dep.name)
+                if prior and prior != dep:
+                    tty.debug("{0} takes priority over {1}"
+                              .format(package_to_spec[dep.name].format(),
+                                      dep.format()))
                 else:
                     package_to_spec[dep.name] = dep
                     spec_list.append(dep)
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index b49bcbcb9b..2828f5806c 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -27,6 +27,7 @@ add         = SpackCommand('add')
 remove      = SpackCommand('remove')
 concretize  = SpackCommand('concretize')
 stage       = SpackCommand('stage')
+uninstall   = SpackCommand('uninstall')
 
 
 def test_add():
@@ -541,9 +542,38 @@ def test_env_commands_die_with_no_env_arg():
     # these have an optional env arg and raise errors via tty.die
     with pytest.raises(spack.main.SpackCommandError):
         env('loads')
-    with pytest.raises(spack.main.SpackCommandError):
-        env('uninstall')
 
     # This should NOT raise an error with no environment
     # it just tells the user there isn't an environment
     env('status')
+
+
+def test_env_blocks_uninstall(mock_stage, mock_fetch, install_mockery):
+    env('create', 'test')
+    with ev.read('test'):
+        add('mpileaks')
+        install('--fake')
+
+    out = uninstall('mpileaks', fail_on_error=False)
+    assert uninstall.returncode == 1
+    assert 'used by the following environments' in out
+
+
+def test_uninstall_removes_from_env(mock_stage, mock_fetch, install_mockery):
+    env('create', 'test')
+    with ev.read('test'):
+        add('mpileaks')
+        add('libelf')
+        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())
+
+    with ev.read('test'):
+        uninstall('-ya')
+
+    test = ev.read('test')
+    assert not test.specs_by_hash
+    assert not test.concretized_order
+    assert not test.user_specs