From 73f7301ec4545b5cce061e8336617f186485121b Mon Sep 17 00:00:00 2001
From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Date: Mon, 2 Mar 2020 11:16:20 -0800
Subject: bugfix: ensure proper dependency handling for package-only installs
(#15197)
The distributed build PR (#13100) -- did not check the install status of dependencies when using the `--only package` option so would refuse to install a package with the claim that it had uninstalled dependencies whether that was the case or not.
- [x] add install status checks for the `--only package` case.
- [x] add initial set of tests
---
lib/spack/spack/installer.py | 77 ++++++++++++++++++++++++++++++++-----
lib/spack/spack/test/cmd/install.py | 24 ++++++++++++
lib/spack/spack/test/installer.py | 65 +++++++++++++++++++++++++++----
3 files changed, 149 insertions(+), 17 deletions(-)
(limited to 'lib')
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index 714dc0f31c..db82844cc5 100755
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -651,6 +651,66 @@ class PackageInstaller(object):
if package_id(comp_pkg) not in self.build_tasks:
self._push_task(comp_pkg, is_compiler, 0, 0, STATUS_ADDED)
+ def _check_db(self, spec):
+ """Determine if the spec is flagged as installed in the database
+
+ Args:
+ spec (Spec): spec whose database install status is being checked
+
+ Return:
+ (rec, installed_in_db) tuple where rec is the database record, or
+ None, if there is no matching spec, and installed_in_db is
+ ``True`` if the spec is considered installed and ``False``
+ otherwise
+ """
+ try:
+ rec = spack.store.db.get_record(spec)
+ installed_in_db = rec.installed if rec else False
+ except KeyError:
+ # KeyError is raised if there is no matching spec in the database
+ # (versus no matching specs that are installed).
+ rec = None
+ installed_in_db = False
+ return rec, installed_in_db
+
+ def _check_deps_status(self):
+ """Check the install status of the explicit spec's dependencies"""
+
+ err = 'Cannot proceed with {0}: {1}'
+ for dep in self.spec.traverse(order='post', root=False):
+ dep_pkg = dep.package
+ dep_id = package_id(dep_pkg)
+
+ # Check for failure since a prefix lock is not required
+ if spack.store.db.prefix_failed(dep):
+ action = "'spack install' the dependency"
+ msg = '{0} is marked as an install failure: {1}' \
+ .format(dep_id, action)
+ raise InstallError(err.format(self.pkg_id, msg))
+
+ # Attempt to get a write lock to ensure another process does not
+ # uninstall the dependency while the requested spec is being
+ # installed
+ ltype, lock = self._ensure_locked('write', dep_pkg)
+ if lock is None:
+ msg = '{0} is write locked by another process'.format(dep_id)
+ raise InstallError(err.format(self.pkg_id, msg))
+
+ # Flag external and upstream packages as being installed
+ if dep_pkg.spec.external or dep_pkg.installed_upstream:
+ form = 'external' if dep_pkg.spec.external else 'upstream'
+ tty.debug('Flagging {0} {1} as installed'.format(form, dep_id))
+ self.installed.add(dep_id)
+ continue
+
+ # Check the database to see if the dependency has been installed
+ # and flag as such if appropriate
+ rec, installed_in_db = self._check_db(dep)
+ if installed_in_db:
+ tty.debug('Flagging {0} as installed per the database'
+ .format(dep_id))
+ self.installed.add(dep_id)
+
def _prepare_for_install(self, task, keep_prefix, keep_stage,
restage=False):
"""
@@ -680,14 +740,7 @@ class PackageInstaller(object):
return
# Determine if the spec is flagged as installed in the database
- try:
- rec = spack.store.db.get_record(task.pkg.spec)
- installed_in_db = rec.installed if rec else False
- except KeyError:
- # KeyError is raised if there is no matching spec in the database
- # (versus no matching specs that are installed).
- rec = None
- installed_in_db = False
+ rec, installed_in_db = self._check_db(task.pkg.spec)
# Make sure the installation directory is in the desired state
# for uninstalled specs.
@@ -929,6 +982,11 @@ class PackageInstaller(object):
# Be sure to clear any previous failure
spack.store.db.clear_failure(self.pkg.spec, force=True)
+ # If not installing dependencies, then determine their
+ # installation status before proceeding
+ if not install_deps:
+ self._check_deps_status()
+
# Now add the package itself, if appropriate
self._push_task(self.pkg, False, 0, 0, STATUS_ADDED)
@@ -1022,7 +1080,8 @@ class PackageInstaller(object):
configure_args = getattr(pkg, attr)()
configure_args = ' '.join(configure_args)
- with open(pkg.configure_args_path, 'w') as args_file:
+ with open(pkg.configure_args_path, 'w') as \
+ args_file:
args_file.write(configure_args)
break
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 2697a539bf..1d4a0c6827 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -632,6 +632,30 @@ def test_install_only_dependencies(tmpdir, mock_fetch, install_mockery):
assert not os.path.exists(root.prefix)
+def test_install_only_package(tmpdir, mock_fetch, install_mockery, capfd):
+ msg = ''
+ with capfd.disabled():
+ try:
+ install('--only', 'package', 'dependent-install')
+ except spack.installer.InstallError as e:
+ msg = str(e)
+
+ assert 'Cannot proceed with dependent-install' in msg
+ assert '1 uninstalled dependency' in msg
+
+
+def test_install_deps_then_package(tmpdir, mock_fetch, install_mockery):
+ dep = Spec('dependency-install').concretized()
+ root = Spec('dependent-install').concretized()
+
+ install('--only', 'dependencies', 'dependent-install')
+ assert os.path.exists(dep.prefix)
+ assert not os.path.exists(root.prefix)
+
+ install('--only', 'package', 'dependent-install')
+ assert os.path.exists(root.prefix)
+
+
@pytest.mark.regression('12002')
def test_install_only_dependencies_in_env(tmpdir, mock_fetch, install_mockery,
mutable_mock_env_path):
diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py
index 7d2eec8caf..eadebfd3e4 100644
--- a/lib/spack/spack/test/installer.py
+++ b/lib/spack/spack/test/installer.py
@@ -27,6 +27,12 @@ def _none(*args, **kwargs):
return None
+def _not_locked(installer, lock_type, pkg):
+ """Generic monkeypatch function for _ensure_locked to return no lock"""
+ tty.msg('{0} locked {1}' .format(lock_type, pkg.spec.name))
+ return lock_type, None
+
+
def _true(*args, **kwargs):
"""Generic monkeypatch function that always returns True."""
return True
@@ -285,6 +291,57 @@ def test_dump_packages_deps(install_mockery, tmpdir):
inst.dump_packages(spec, '.')
+@pytest.mark.tld
+def test_check_deps_status_errs(install_mockery, monkeypatch):
+ """Test to cover _check_deps_status failures."""
+ spec, installer = create_installer('a')
+
+ # Make sure the package is identified as failed
+ orig_fn = spack.database.Database.prefix_failed
+ monkeypatch.setattr(spack.database.Database, 'prefix_failed', _true)
+
+ with pytest.raises(inst.InstallError, matches='install failure'):
+ installer._check_deps_status()
+
+ monkeypatch.setattr(spack.database.Database, 'prefix_failed', orig_fn)
+
+ # Ensure do not acquire the lock
+ monkeypatch.setattr(inst.PackageInstaller, '_ensure_locked', _not_locked)
+
+ with pytest.raises(inst.InstallError, matches='write locked by another'):
+ installer._check_deps_status()
+
+
+@pytest.mark.tld
+def test_check_deps_status_external(install_mockery, monkeypatch):
+ """Test to cover _check_deps_status for external."""
+ spec, installer = create_installer('a')
+
+ deps = spec.dependencies()
+ assert len(deps) > 0
+ dep_id = 'b'
+
+ # Ensure the known dependent is installed if flagged as external
+ monkeypatch.setattr(spack.spec.Spec, 'external', True)
+ installer._check_deps_status()
+ assert dep_id in installer.installed
+
+
+@pytest.mark.tld
+def test_check_deps_status_upstream(install_mockery, monkeypatch):
+ """Test to cover _check_deps_status for upstream."""
+ spec, installer = create_installer('a')
+
+ deps = spec.dependencies()
+ assert len(deps) > 0
+ dep_id = 'b'
+
+ # Ensure the known dependent, b, is installed if flagged as upstream
+ monkeypatch.setattr(spack.package.PackageBase, 'installed_upstream', True)
+ installer._check_deps_status()
+ assert dep_id in installer.installed
+
+
def test_add_bootstrap_compilers(install_mockery, monkeypatch):
"""Test to cover _add_bootstrap_compilers."""
def _pkgs(pkg):
@@ -458,10 +515,6 @@ def test_install_lock_failures(install_mockery, monkeypatch, capfd):
def _requeued(installer, task):
tty.msg('requeued {0}' .format(task.pkg.spec.name))
- def _not_locked(installer, lock_type, pkg):
- tty.msg('{0} locked {1}' .format(lock_type, pkg.spec.name))
- return lock_type, None
-
spec, installer = create_installer('b')
# Ensure never acquire a lock
@@ -485,10 +538,6 @@ def test_install_lock_installed_requeue(install_mockery, monkeypatch, capfd):
def _install(installer, task, **kwargs):
tty.msg('{0} installing'.format(task.pkg.spec.name))
- def _not_locked(installer, lock_type, pkg):
- tty.msg('{0} locked {1}' .format(lock_type, pkg.spec.name))
- return lock_type, None
-
def _prep(installer, task, keep_prefix, keep_stage, restage):
installer.installed.add('b')
tty.msg('{0} is installed' .format(task.pkg.spec.name))
--
cgit v1.2.3-70-g09d2