spack uninstall no longer requires a known package. (#3915)

- Spack install would previously fail if it could not load a package for
  the thing being uninstalled.

- This reworks uninstall to handle cases where the package is no longer
  known, e.g.:
    a) the package has been renamed or is no longer in Spack
    b) the repository the package came from is no longer registered in
       repos.yaml

6 files changed, 102 insertions, 82 deletions

diff --git a/lib/spack/spack/cmd/uninstall.py b/lib/spack/spack/cmd/uninstall.py
index e0b40e0627..f3eaddf88a 100644
--- a/lib/spack/spack/cmd/uninstall.py
+++ b/lib/spack/spack/cmd/uninstall.py
@@ -76,9 +76,9 @@ def setup_parser(subparser):
         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
+def find_matching_specs(specs, allow_multiple_matches=False, force=False):
+    """Returns a list of specs matching the not necessarily
+       concretized specs given from cli
 
     Args:
         specs: list of specs to be matched against installed packages
@@ -147,10 +147,10 @@ def do_uninstall(specs, force):
         try:
             # should work if package is known to spack
             packages.append(item.package)
-        except spack.repository.UnknownPackageError:
+        except spack.repository.UnknownEntityError:
             # The package.py file has gone away -- but still
             # want to uninstall.
-            spack.Package(item).do_uninstall(force=True)
+            spack.Package.uninstall_by_spec(item, force=True)
 
     # Sort packages to be uninstalled by the number of installed dependents
     # This ensures we do things in the right order
@@ -169,7 +169,7 @@ def get_uninstall_list(args):
 
     # Gets the list of installed specs that match the ones give via cli
     # takes care of '-a' is given in the cli
-    uninstall_list = concretize_specs(specs, args.all, args.force)
+    uninstall_list = find_matching_specs(specs, args.all, args.force)
 
     # Takes care of '-d'
     dependent_list = installed_dependents(uninstall_list)
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index c63da4cf2e..3cb8ced342 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -40,7 +40,9 @@ filesystem.
 
 """
 import os
+import sys
 import socket
+import contextlib
 from six import string_types
 from six import iteritems
 
@@ -52,12 +54,13 @@ from llnl.util.lock import *
 
 import spack.store
 import spack.repository
-from spack.directory_layout import DirectoryLayoutError
-from spack.version import Version
 import spack.spec
-from spack.error import SpackError
 import spack.util.spack_yaml as syaml
 import spack.util.spack_json as sjson
+from spack.util.crypto import bit_length
+from spack.directory_layout import DirectoryLayoutError
+from spack.error import SpackError
+from spack.version import Version
 
 
 # DB goes in this directory underneath the root
@@ -127,6 +130,9 @@ class InstallRecord(object):
 
 class Database(object):
 
+    """Per-process lock objects for each install prefix."""
+    _prefix_locks = {}
+
     def __init__(self, root, db_dir=None):
         """Create a Database for Spack installations under ``root``.
 
@@ -185,6 +191,47 @@ class Database(object):
         """Get a read lock context manager for use in a `with` block."""
         return ReadTransaction(self.lock, self._read, timeout=timeout)
 
+    def prefix_lock(self, spec):
+        """Get a lock on a particular spec's installation directory.
+
+        NOTE: The installation directory **does not** need to exist.
+
+        Prefix lock is a byte range lock on the nth byte of a file.
+
+        The lock file is ``spack.store.db.prefix_lock`` -- the DB
+        tells us what to call it and it lives alongside the install DB.
+
+        n is the sys.maxsize-bit prefix of the DAG hash.  This makes
+        likelihood of collision is very low AND it gives us
+        readers-writer lock semantics with just a single lockfile, so no
+        cleanup required.
+        """
+        prefix = spec.prefix
+        if prefix not in self._prefix_locks:
+            self._prefix_locks[prefix] = Lock(
+                self.prefix_lock_path,
+                spec.dag_hash_bit_prefix(bit_length(sys.maxsize)), 1)
+
+        return self._prefix_locks[prefix]
+
+    @contextlib.contextmanager
+    def prefix_read_lock(self, spec):
+        prefix_lock = self.prefix_lock(spec)
+        try:
+            prefix_lock.acquire_read(60)
+            yield self
+        finally:
+            prefix_lock.release_read()
+
+    @contextlib.contextmanager
+    def prefix_write_lock(self, spec):
+        prefix_lock = self.prefix_lock(spec)
+        try:
+            prefix_lock.acquire_write(60)
+            yield self
+        finally:
+            prefix_lock.release_write()
+
     def _write_to_file(self, stream):
         """Write out the databsae to a JSON file.
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 108ddeff07..0402926590 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -46,7 +46,6 @@ from six import StringIO
 from six import string_types
 from six import with_metaclass
 
-import llnl.util.lock
 import llnl.util.tty as tty
 import spack
 import spack.store
@@ -66,7 +65,6 @@ from llnl.util.link_tree import LinkTree
 from llnl.util.tty.log import log_output
 from spack import directory_layout
 from spack.stage import Stage, ResourceStage, StageComposite
-from spack.util.crypto import bit_length
 from spack.util.environment import dump_environment
 from spack.version import *
 
@@ -513,16 +511,10 @@ class PackageBase(with_metaclass(PackageMeta, object)):
     """
     sanity_check_is_dir = []
 
-    """Per-process lock objects for each install prefix."""
-    prefix_locks = {}
-
     def __init__(self, spec):
         # this determines how the package should be built.
         self.spec = spec
 
-        # Lock on the prefix shared resource. Will be set in prefix property
-        self._prefix_lock = None
-
         # Name of package is the name of its module, without the
         # containing module names.
         self.name = self.module.__name__
@@ -859,29 +851,6 @@ class PackageBase(with_metaclass(PackageMeta, object)):
         return os.path.isdir(self.prefix)
 
     @property
-    def prefix_lock(self):
-        """Prefix lock is a byte range lock on the nth byte of a file.
-
-        The lock file is ``spack.store.db.prefix_lock`` -- the DB
-        tells us what to call it and it lives alongside the install DB.
-
-        n is the sys.maxsize-bit prefix of the DAG hash.  This makes
-        likelihood of collision is very low AND it gives us
-        readers-writer lock semantics with just a single lockfile, so no
-        cleanup required.
-        """
-        if self._prefix_lock is None:
-            prefix = self.spec.prefix
-            if prefix not in Package.prefix_locks:
-                Package.prefix_locks[prefix] = llnl.util.lock.Lock(
-                    spack.store.db.prefix_lock_path,
-                    self.spec.dag_hash_bit_prefix(bit_length(sys.maxsize)), 1)
-
-            self._prefix_lock = Package.prefix_locks[prefix]
-
-        return self._prefix_lock
-
-    @property
     def prefix(self):
         """Get the prefix into which this package should be installed."""
         return self.spec.prefix
@@ -1106,26 +1075,10 @@ class PackageBase(with_metaclass(PackageMeta, object)):
         return resource_stage_folder
 
     @contextlib.contextmanager
-    def _prefix_read_lock(self):
-        try:
-            self.prefix_lock.acquire_read(60)
-            yield self
-        finally:
-            self.prefix_lock.release_read()
-
-    @contextlib.contextmanager
-    def _prefix_write_lock(self):
-        try:
-            self.prefix_lock.acquire_write(60)
-            yield self
-        finally:
-            self.prefix_lock.release_write()
-
-    @contextlib.contextmanager
     def _stage_and_write_lock(self):
         """Prefix lock nested in a stage."""
         with self.stage:
-            with self._prefix_write_lock():
+            with spack.store.db.prefix_write_lock(self.spec):
                 yield
 
     def do_install(self,
@@ -1176,8 +1129,12 @@ class PackageBase(with_metaclass(PackageMeta, object)):
 
         # Ensure package is not already installed
         layout = spack.store.layout
-        with self._prefix_read_lock():
-            if layout.check_installed(self.spec):
+        with spack.store.db.prefix_read_lock(self.spec):
+            if (keep_prefix and os.path.isdir(self.prefix) and
+                    (not self.installed)):
+                tty.msg(
+                    "Continuing from partial install of %s" % self.name)
+            elif layout.check_installed(self.spec):
                 tty.msg(
                     "%s is already installed in %s" % (self.name, self.prefix))
                 rec = spack.store.db.get_record(self.spec)
@@ -1247,7 +1204,6 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             )
 
             self.stage.keep = keep_stage
-
             with self._stage_and_write_lock():
                 # Run the pre-install hook in the child process after
                 # the directory is created.
@@ -1503,34 +1459,50 @@ class PackageBase(with_metaclass(PackageMeta, object)):
         """
         pass
 
-    def do_uninstall(self, force=False):
-        if not self.installed:
+    @staticmethod
+    def uninstall_by_spec(spec, force=False):
+        if not os.path.isdir(spec.prefix):
             # prefix may not exist, but DB may be inconsistent. Try to fix by
             # removing, but omit hooks.
-            specs = spack.store.db.query(self.spec, installed=True)
+            specs = spack.store.db.query(spec, installed=True)
             if specs:
                 spack.store.db.remove(specs[0])
-                tty.msg("Removed stale DB entry for %s" % self.spec.short_spec)
+                tty.msg("Removed stale DB entry for %s" % spec.short_spec)
                 return
             else:
-                raise InstallError(str(self.spec) + " is not installed.")
+                raise InstallError(str(spec) + " is not installed.")
 
         if not force:
-            dependents = spack.store.db.installed_dependents(self.spec)
+            dependents = spack.store.db.installed_dependents(spec)
             if dependents:
-                raise PackageStillNeededError(self.spec, dependents)
+                raise PackageStillNeededError(spec, dependents)
+
+        # Try to get the pcakage for the spec
+        try:
+            pkg = spec.package
+        except spack.repository.UnknownEntityError:
+            pkg = None
 
         # Pre-uninstall hook runs first.
-        with self._prefix_write_lock():
-            spack.hooks.pre_uninstall(self)
+        with spack.store.db.prefix_write_lock(spec):
+            # TODO: hooks should take specs, not packages.
+            if pkg is not None:
+                spack.hooks.pre_uninstall(pkg)
+
             # Uninstalling in Spack only requires removing the prefix.
-            self.remove_prefix()
-            #
-            spack.store.db.remove(self.spec)
-        tty.msg("Successfully uninstalled %s" % self.spec.short_spec)
+            spack.store.layout.remove_install_directory(spec)
+            spack.store.db.remove(spec)
 
-        # Once everything else is done, run post install hooks
-        spack.hooks.post_uninstall(self)
+            # TODO: refactor hooks to use specs, not packages.
+            if pkg is not None:
+                spack.hooks.post_uninstall(pkg)
+
+        tty.msg("Successfully uninstalled %s" % spec.short_spec)
+
+    def do_uninstall(self, force=False):
+        """Uninstall this package by spec."""
+        # delegate to instance-less method.
+        Package.uninstall_by_spec(self.spec, force)
 
     def _check_extendable(self):
         if not self.extendable:
diff --git a/lib/spack/spack/repository.py b/lib/spack/spack/repository.py
index 5486f7a9a4..3e43cce781 100644
--- a/lib/spack/spack/repository.py
+++ b/lib/spack/spack/repository.py
@@ -924,11 +924,11 @@ class DuplicateRepoError(RepoError):
     """Raised when duplicate repos are added to a RepoPath."""
 
 
-class PackageLoadError(spack.error.SpackError):
-    """Superclass for errors related to loading packages."""
+class UnknownEntityError(RepoError):
+    """Raised when we encounter a package spack doesn't have."""
 
 
-class UnknownPackageError(PackageLoadError):
+class UnknownPackageError(UnknownEntityError):
     """Raised when we encounter a package spack doesn't have."""
 
     def __init__(self, name, repo=None):
@@ -941,7 +941,7 @@ class UnknownPackageError(PackageLoadError):
         self.name = name
 
 
-class UnknownNamespaceError(PackageLoadError):
+class UnknownNamespaceError(UnknownEntityError):
     """Raised when we encounter an unknown namespace"""
 
     def __init__(self, namespace):
@@ -949,7 +949,7 @@ class UnknownNamespaceError(PackageLoadError):
             "Unknown namespace: %s" % namespace)
 
 
-class FailedConstructorError(PackageLoadError):
+class FailedConstructorError(RepoError):
     """Raised when a package's class constructor fails."""
 
     def __init__(self, name, exc_type, exc_obj, exc_tb):
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index adddb87428..b4170a0f7a 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2187,7 +2187,7 @@ class Spec(object):
         if not self.virtual and other.virtual:
             try:
                 pkg = spack.repo.get(self.fullname)
-            except spack.repository.PackageLoadError:
+            except spack.repository.UnknownEntityError:
                 # If we can't get package info on this spec, don't treat
                 # it as a provider of this vdep.
                 return False
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 29b6882927..086c4c90f5 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -222,7 +222,8 @@ class Stage(object):
         self.keep = keep
 
         # File lock for the stage directory.  We use one file for all
-        # stage locks. See Spec.prefix_lock for details on this approach.
+        # stage locks. See spack.database.Database.prefix_lock for
+        # details on this approach.
         self._lock = None
         if lock:
             if self.name not in Stage.stage_locks: