From ead58cbb9062fab01363e35746e65478acc2dcf5 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Fri, 21 Apr 2017 16:52:44 -0700 Subject: 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 | 12 ++--- lib/spack/spack/database.py | 53 ++++++++++++++++++-- lib/spack/spack/package.py | 104 ++++++++++++++------------------------- lib/spack/spack/repository.py | 10 ++-- lib/spack/spack/spec.py | 2 +- lib/spack/spack/stage.py | 3 +- 6 files changed, 102 insertions(+), 82 deletions(-) (limited to 'lib') 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__ @@ -858,29 +850,6 @@ class PackageBase(with_metaclass(PackageMeta, object)): 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 @@ class PackageBase(with_metaclass(PackageMeta, object)): 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 @@ 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: -- cgit v1.2.3-60-g2f50