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(-) 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-60-g2f50