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

lib/spack/spack/cmd/uninstall.py

@@ -76,8 +76,8 @@ def setup_parser(subparser):
         help="specs of packages to uninstall")
 
 
-def concretize_specs(specs, allow_multiple_matches=False, force=False):
+def find_matching_specs(specs, allow_multiple_matches=False, force=False):
-    """Returns a list of specs matching the non necessarily
+    """Returns a list of specs matching the not necessarily
        concretized specs given from cli
 
     Args:
@@ -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)

lib/spack/spack/database.py

@@ -40,7 +40,9 @@
 
 """
 import os
+import sys
 import socket
+import contextlib
 from six import string_types
 from six import iteritems
 
@@ -52,12 +54,13 @@
 
 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 @@ def from_dict(cls, spec, dictionary):
 
 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 @@ def read_transaction(self, timeout=_db_lock_timeout):
         """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.
 

lib/spack/spack/package.py

@@ -46,7 +46,6 @@
 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.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 SomePackage(Package):
     """
     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__
@@ -858,29 +850,6 @@ def provides(self, vpkg_name):
     def installed(self):
         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."""
@@ -1105,27 +1074,11 @@ def _resource_stage(self, resource):
         resource_stage_folder = '-'.join(pieces)
         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 @@ def do_install(self,
 
         # Ensure package is not already installed
         layout = spack.store.layout
-        with self._prefix_read_lock():
+        with spack.store.db.prefix_read_lock(self.spec):
-            if layout.check_installed(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 @@ def build_process(input_stream):
             )
 
             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 @@ def setup_dependent_package(self, module, dependent_spec):
         """
         pass
 
-    def do_uninstall(self, force=False):
+    @staticmethod
-        if not self.installed:
+    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():
+        with spack.store.db.prefix_write_lock(spec):
-            spack.hooks.pre_uninstall(self)
+            # TODO: hooks should take specs, not packages.
-            # Uninstalling in Spack only requires removing the prefix.
+            if pkg is not None:
-            self.remove_prefix()
+                spack.hooks.pre_uninstall(pkg)
-            #
-            spack.store.db.remove(self.spec)
-        tty.msg("Successfully uninstalled %s" % self.spec.short_spec)
 
-        # Once everything else is done, run post install hooks
+            # Uninstalling in Spack only requires removing the prefix.
-        spack.hooks.post_uninstall(self)
+            spack.store.layout.remove_install_directory(spec)
+            spack.store.db.remove(spec)
+
+            # 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:

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):
+class UnknownEntityError(RepoError):
-    """Superclass for errors related to loading packages."""
+    """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 @@ def __init__(self, name, repo=None):
         self.name = name
 
 
-class UnknownNamespaceError(PackageLoadError):
+class UnknownNamespaceError(UnknownEntityError):
     """Raised when we encounter an unknown namespace"""
 
     def __init__(self, namespace):
@@ -949,7 +949,7 @@ def __init__(self, namespace):
             "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):

lib/spack/spack/spec.py

@@ -2187,7 +2187,7 @@ def satisfies(self, other, deps=True, strict=False, strict_deps=False):
         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

lib/spack/spack/stage.py

@@ -222,7 +222,8 @@ def __init__(
         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: