Merge pull request #670 from epfl-scitas/uninstall_improved

enhancement : recursive uninstallation of dependent packages

7 files changed, 290 insertions, 143 deletions

diff --git a/lib/spack/docs/basic_usage.rst b/lib/spack/docs/basic_usage.rst
index accf09cc2a..72a02802fb 100644
--- a/lib/spack/docs/basic_usage.rst
+++ b/lib/spack/docs/basic_usage.rst
@@ -149,26 +149,46 @@ customize an installation in :ref:`sec-specs`.
 ``spack uninstall``
 ~~~~~~~~~~~~~~~~~~~~~
 
-To uninstall a package, type ``spack uninstall <package>``.  This will
-completely remove the directory in which the package was installed.
+To uninstall a package, type ``spack uninstall <package>``.  This will ask the user for
+confirmation, and in case will completely remove the directory in which the package was installed.
 
 .. code-block:: sh
 
    spack uninstall mpich
 
 If there are still installed packages that depend on the package to be
-uninstalled, spack will refuse to uninstall it. You can override this
-behavior with ``spack uninstall -f <package>``, but you risk breaking
-other installed packages. In general, it is safer to remove dependent
-packages *before* removing their dependencies.
+uninstalled, spack will refuse to uninstall it.
 
-A line like ``spack uninstall mpich`` may be ambiguous, if multiple
-``mpich`` configurations are installed.  For example, if both
+To uninstall a package and every package that depends on it, you may give the
+`--dependents` option.
+
+.. code-block:: sh
+
+   spack uninstall --dependents mpich
+
+will display a list of all the packages that depends on `mpich` and, upon confirmation,
+will uninstall them in the right order.
+
+A line like
+
+.. code-block:: sh
+
+   spack uninstall mpich
+
+may be ambiguous, if multiple ``mpich`` configurations are installed.  For example, if both
 ``mpich@3.0.2`` and ``mpich@3.1`` are installed, ``mpich`` could refer
 to either one. Because it cannot determine which one to uninstall,
-Spack will ask you to provide a version number to remove the
-ambiguity.  As an example, ``spack uninstall mpich@3.1`` is
-unambiguous in this scenario.
+Spack will ask you either to provide a version number to remove the
+ambiguity or use the ``--all`` option to uninstall all of the matching packages.
+
+You may force uninstall a package with the `--force` option
+
+.. code-block:: sh
+
+   spack uninstall --force mpich
+
+but you risk breaking other installed packages. In general, it is safer to remove dependent
+packages *before* removing their dependencies or use the `--dependents` option.
 
 
 Seeing installed packages
diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py
index 350ef372cb..1ff3d8db5f 100644
--- a/lib/spack/spack/cmd/uninstall.py
+++ b/lib/spack/spack/cmd/uninstall.py
@@ -23,19 +23,33 @@
 # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
 from __future__ import print_function
-import sys
+
 import argparse
 
 import llnl.util.tty as tty
-from llnl.util.tty.colify import colify
-
 import spack
 import spack.cmd
 import spack.repository
 from spack.cmd.find import display_specs
-from spack.package import PackageStillNeededError
 
-description="Remove an installed package"
+description = "Remove an installed package"
+
+error_message = """You can either:
+    a) Use a more specific spec, or
+    b) use spack uninstall -a to uninstall ALL matching specs.
+"""
+
+
+def ask_for_confirmation(message):
+    while True:
+        tty.msg(message + '[y/n]')
+        choice = raw_input().lower()
+        if choice == 'y':
+            break
+        elif choice == 'n':
+            raise SystemExit('Operation aborted')
+        tty.warn('Please reply either "y" or "n"')
+
 
 def setup_parser(subparser):
     subparser.add_argument(
@@ -44,10 +58,101 @@ def setup_parser(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 say uninstall libelf, ALL versions of " +
-        "libelf are uninstalled. This is both useful and dangerous, like rm -r.")
+             "supplied spec. i.e., if you say uninstall libelf, ALL versions of " +
+             "libelf are uninstalled. This is both useful and dangerous, like rm -r.")
     subparser.add_argument(
-        'packages', nargs=argparse.REMAINDER, help="specs of packages to uninstall")
+        '-d', '--dependents', action='store_true', dest='dependents',
+        help='Also uninstall any packages that depend on the ones given via command line.'
+    )
+    subparser.add_argument(
+        '-y', '--yes-to-all', action='store_true', dest='yes_to_all',
+        help='Assume "yes" is the answer to every confirmation asked to the user.'
+
+    )
+    subparser.add_argument('packages', nargs=argparse.REMAINDER, help="specs of packages to uninstall")
+
+
+def concretize_specs(specs, allow_multiple_matches=False, force=False):
+    """
+    Returns a list of specs matching the non necessarily concretized specs given from cli
+
+    Args:
+        specs: list of specs to be matched against installed packages
+        allow_multiple_matches : boolean (if True multiple matches for each item in specs are admitted)
+
+    Return:
+        list of specs
+    """
+    specs_from_cli = []  # List of specs that match expressions given via command line
+    has_errors = False
+    for spec in specs:
+        matching = spack.installed_db.query(spec)
+        # 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:
+            tty.error("%s matches multiple packages:" % spec)
+            print()
+            display_specs(matching, long=True)
+            print()
+            has_errors = True
+
+        # No installed package matches the query
+        if len(matching) == 0 and not force:
+            tty.error("%s does not match any installed packages." % spec)
+            has_errors = True
+
+        specs_from_cli.extend(matching)
+    if has_errors:
+        tty.die(error_message)
+
+    return specs_from_cli
+
+
+def installed_dependents(specs):
+    """
+    Returns a dictionary that maps a spec with a list of its installed dependents
+
+    Args:
+        specs: list of specs to be checked for dependents
+
+    Returns:
+        dictionary of installed dependents
+    """
+    dependents = {}
+    for item in specs:
+        lst = [x for x in item.package.installed_dependents if x not in specs]
+        if lst:
+            lst = list(set(lst))
+            dependents[item] = lst
+    return dependents
+
+
+def do_uninstall(specs, force):
+    """
+    Uninstalls all the specs in a list.
+
+    Args:
+        specs: list of specs to be uninstalled
+        force: force uninstallation (boolean)
+    """
+    packages = []
+    for item in specs:
+        try:
+            # should work if package is known to spack
+            packages.append(item.package)
+        except spack.repository.UnknownPackageError as e:
+            # The package.py file has gone away -- but still
+            # want to uninstall.
+            spack.Package(item).do_uninstall(force=True)
+
+    # 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):
+        return len(pkg.installed_dependents)
+
+    packages.sort(key=num_installed_deps)
+    for item in packages:
+        item.do_uninstall(force=force)
 
 
 def uninstall(parser, args):
@@ -56,50 +161,34 @@ def uninstall(parser, args):
 
     with spack.installed_db.write_transaction():
         specs = spack.cmd.parse_specs(args.packages)
+        # Gets the list of installed specs that match the ones give via cli
+        uninstall_list = concretize_specs(specs, args.all, args.force)  # takes care of '-a' is given in the cli
+        dependent_list = installed_dependents(uninstall_list)  # takes care of '-d'
 
-        # For each spec provided, make sure it refers to only one package.
-        # Fail and ask user to be unambiguous if it doesn't
-        pkgs = []
-        for spec in specs:
-            matching_specs = spack.installed_db.query(spec)
-            if not args.all and len(matching_specs) > 1:
-                tty.error("%s matches multiple packages:" % spec)
-                print()
-                display_specs(matching_specs, long=True)
-                print()
-                print("You can either:")
-                print("  a) Use a more specific spec, or")
-                print("  b) use spack uninstall -a to uninstall ALL matching specs.")
-                sys.exit(1)
-
-            if len(matching_specs) == 0:
-                if args.force: continue
-                tty.die("%s does not match any installed packages." % spec)
-
-            for s in matching_specs:
-                try:
-                    # should work if package is known to spack
-                    pkgs.append(s.package)
-                except spack.repository.UnknownPackageError as e:
-                    # The package.py file has gone away -- but still
-                    # want to uninstall.
-                    spack.Package(s).do_uninstall(force=True)
-
-        # 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):
-            return len(pkg.installed_dependents)
-        pkgs.sort(key=num_installed_deps)
-
-        # Uninstall packages in order now.
-        for pkg in pkgs:
-            try:
-                pkg.do_uninstall(force=args.force)
-            except PackageStillNeededError as e:
-                tty.error("Will not uninstall %s" % e.spec.format("$_$@$%@$#", color=True))
+        # 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.format("$_$@$%@$#", color=True))
                 print('')
                 print("The following packages depend on it:")
-                display_specs(e.dependents, long=True)
+                display_specs(lst, long=True)
                 print('')
-                print("You can use spack uninstall -f to force this action.")
-                sys.exit(1)
+                has_error = True
+        elif args.dependents:
+            for key, lst in dependent_list.items():
+                uninstall_list.extend(lst)
+            uninstall_list = list(set(uninstall_list))
+
+        if has_error:
+            tty.die('You can use spack uninstall --dependents to uninstall these dependencies as well')
+
+        if not args.yes_to_all:
+            tty.msg("The following packages will be uninstalled : ")
+            print('')
+            display_specs(uninstall_list, long=True)
+            print('')
+            ask_for_confirmation('Do you want to proceed ? ')
+
+        # Uninstall everything on the list
+        do_uninstall(uninstall_list, args.force)
diff --git a/lib/spack/spack/test/__init__.py b/lib/spack/spack/test/__init__.py
index cd842561e6..175a49428c 100644
--- a/lib/spack/spack/test/__init__.py
+++ b/lib/spack/spack/test/__init__.py
@@ -67,7 +67,8 @@ test_names = ['versions',
               'namespace_trie',
               'yaml',
               'sbang',
-              'environment']
+              'environment',
+              'cmd.uninstall']
 
 
 def list_tests():
diff --git a/lib/spack/spack/test/cmd/__init__.py b/lib/spack/spack/test/cmd/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/lib/spack/spack/test/cmd/__init__.py
diff --git a/lib/spack/spack/test/cmd/uninstall.py b/lib/spack/spack/test/cmd/uninstall.py
new file mode 100644
index 0000000000..80efe06d36
--- /dev/null
+++ b/lib/spack/spack/test/cmd/uninstall.py
@@ -0,0 +1,37 @@
+import spack.test.mock_database
+
+from spack.cmd.uninstall import uninstall
+
+
+class MockArgs(object):
+    def __init__(self, packages, all=False, force=False, dependents=False):
+        self.packages = packages
+        self.all = all
+        self.force = force
+        self.dependents = dependents
+        self.yes_to_all = True
+
+
+class TestUninstall(spack.test.mock_database.MockDatabase):
+    def test_uninstall(self):
+        parser = None
+        # Multiple matches
+        args = MockArgs(['mpileaks'])
+        self.assertRaises(SystemExit, uninstall, parser, args)
+        # Installed dependents
+        args = MockArgs(['libelf'])
+        self.assertRaises(SystemExit, uninstall, parser, args)
+        # Recursive uninstall
+        args = MockArgs(['callpath'], all=True, dependents=True)
+        uninstall(parser, args)
+
+        all_specs = spack.install_layout.all_specs()
+        self.assertEqual(len(all_specs), 7)
+        # query specs with multiple configurations
+        mpileaks_specs = [s for s in all_specs if s.satisfies('mpileaks')]
+        callpath_specs = [s for s in all_specs if s.satisfies('callpath')]
+        mpi_specs = [s for s in all_specs if s.satisfies('mpi')]
+
+        self.assertEqual(len(mpileaks_specs), 0)
+        self.assertEqual(len(callpath_specs), 0)
+        self.assertEqual(len(mpi_specs),      3)
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index ce6e8a0552..465263d057 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -28,16 +28,12 @@ both in memory and in its file
 """
 import os.path
 import multiprocessing
-import shutil
-import tempfile
 
 import spack
 from llnl.util.filesystem import join_path
 from llnl.util.lock import *
 from llnl.util.tty.colify import colify
-from spack.database import Database
-from spack.directory_layout import YamlDirectoryLayout
-from spack.test.mock_packages_test import *
+from spack.test.mock_database import MockDatabase
 
 
 def _print_ref_counts():
@@ -75,80 +71,7 @@ def _print_ref_counts():
     colify(recs, cols=3)
 
 
-class DatabaseTest(MockPackagesTest):
-
-    def _mock_install(self, spec):
-        s = Spec(spec)
-        s.concretize()
-        pkg = spack.repo.get(s)
-        pkg.do_install(fake=True)
-
-
-    def _mock_remove(self, spec):
-        specs = spack.installed_db.query(spec)
-        assert(len(specs) == 1)
-        spec = specs[0]
-        spec.package.do_uninstall(spec)
-
-
-    def setUp(self):
-        super(DatabaseTest, self).setUp()
-        #
-        # TODO: make the mockup below easier.
-        #
-
-        # Make a fake install directory
-        self.install_path = tempfile.mkdtemp()
-        self.spack_install_path = spack.install_path
-        spack.install_path = self.install_path
-
-        self.install_layout = YamlDirectoryLayout(self.install_path)
-        self.spack_install_layout = spack.install_layout
-        spack.install_layout = self.install_layout
-
-        # Make fake database and fake install directory.
-        self.installed_db = Database(self.install_path)
-        self.spack_installed_db = spack.installed_db
-        spack.installed_db = self.installed_db
-
-        # make a mock database with some packages installed note that
-        # the ref count for dyninst here will be 3, as it's recycled
-        # across each install.
-        #
-        # Here is what the mock DB looks like:
-        #
-        # o  mpileaks     o  mpileaks'    o  mpileaks''
-        # |\              |\              |\
-        # | o  callpath   | o  callpath'  | o  callpath''
-        # |/|             |/|             |/|
-        # o |  mpich      o |  mpich2     o |  zmpi
-        #   |               |             o |  fake
-        #   |               |               |
-        #   |               |______________/
-        #   | .____________/
-        #   |/
-        #   o  dyninst
-        #   |\
-        #   | o  libdwarf
-        #   |/
-        #   o  libelf
-        #
-
-        # Transaction used to avoid repeated writes.
-        with spack.installed_db.write_transaction():
-            self._mock_install('mpileaks ^mpich')
-            self._mock_install('mpileaks ^mpich2')
-            self._mock_install('mpileaks ^zmpi')
-
-
-    def tearDown(self):
-        super(DatabaseTest, self).tearDown()
-        shutil.rmtree(self.install_path)
-        spack.install_path = self.spack_install_path
-        spack.install_layout = self.spack_install_layout
-        spack.installed_db = self.spack_installed_db
-
-
+class DatabaseTest(MockDatabase):
     def test_005_db_exists(self):
         """Make sure db cache file exists after creating."""
         index_file = join_path(self.install_path, '.spack-db', 'index.yaml')
@@ -157,7 +80,6 @@ class DatabaseTest(MockPackagesTest):
         self.assertTrue(os.path.exists(index_file))
         self.assertTrue(os.path.exists(lock_file))
 
-
     def test_010_all_install_sanity(self):
         """Ensure that the install layout reflects what we think it does."""
         all_specs = spack.install_layout.all_specs()
diff --git a/lib/spack/spack/test/mock_database.py b/lib/spack/spack/test/mock_database.py
new file mode 100644
index 0000000000..6fd05439bf
--- /dev/null
+++ b/lib/spack/spack/test/mock_database.py
@@ -0,0 +1,78 @@
+import shutil
+import tempfile
+
+import spack
+from spack.spec import Spec
+from spack.database import Database
+from spack.directory_layout import YamlDirectoryLayout
+from spack.test.mock_packages_test import MockPackagesTest
+
+
+class MockDatabase(MockPackagesTest):
+    def _mock_install(self, spec):
+        s = Spec(spec)
+        s.concretize()
+        pkg = spack.repo.get(s)
+        pkg.do_install(fake=True)
+
+    def _mock_remove(self, spec):
+        specs = spack.installed_db.query(spec)
+        assert(len(specs) == 1)
+        spec = specs[0]
+        spec.package.do_uninstall(spec)
+
+    def setUp(self):
+        super(MockDatabase, self).setUp()
+        #
+        # TODO: make the mockup below easier.
+        #
+
+        # Make a fake install directory
+        self.install_path = tempfile.mkdtemp()
+        self.spack_install_path = spack.install_path
+        spack.install_path = self.install_path
+
+        self.install_layout = YamlDirectoryLayout(self.install_path)
+        self.spack_install_layout = spack.install_layout
+        spack.install_layout = self.install_layout
+
+        # Make fake database and fake install directory.
+        self.installed_db = Database(self.install_path)
+        self.spack_installed_db = spack.installed_db
+        spack.installed_db = self.installed_db
+
+        # make a mock database with some packages installed note that
+        # the ref count for dyninst here will be 3, as it's recycled
+        # across each install.
+        #
+        # Here is what the mock DB looks like:
+        #
+        # o  mpileaks     o  mpileaks'    o  mpileaks''
+        # |\              |\              |\
+        # | o  callpath   | o  callpath'  | o  callpath''
+        # |/|             |/|             |/|
+        # o |  mpich      o |  mpich2     o |  zmpi
+        #   |               |             o |  fake
+        #   |               |               |
+        #   |               |______________/
+        #   | .____________/
+        #   |/
+        #   o  dyninst
+        #   |\
+        #   | o  libdwarf
+        #   |/
+        #   o  libelf
+        #
+
+        # Transaction used to avoid repeated writes.
+        with spack.installed_db.write_transaction():
+            self._mock_install('mpileaks ^mpich')
+            self._mock_install('mpileaks ^mpich2')
+            self._mock_install('mpileaks ^zmpi')
+
+    def tearDown(self):
+        super(MockDatabase, self).tearDown()
+        shutil.rmtree(self.install_path)
+        spack.install_path = self.spack_install_path
+        spack.install_layout = self.spack_install_layout
+        spack.installed_db = self.spack_installed_db