From 1e69d9d1a94c1f3f5b165e7b5eeebcf65e1789fc Mon Sep 17 00:00:00 2001 From: scheibelp Date: Tue, 13 Jun 2017 09:15:51 -0700 Subject: Override partial installs by default - part three (#4331) * During install, remove prior unfinished installs If a user performs an installation which fails, in some cases the install prefix is still present, and the stage path may also be present. With this commit, unless the user specifies '--keep-prefix', installs are guaranteed to begin with a clean slate. The database is used to decide whether an install finished, since a database record is not added until the end of the install process. * test updates * repair_partial uses keep_prefix and keep_stage * use of mock stage object to ensure that stage is destroyed when it should be destroyed (and otherwise not) * add --restage option to 'install' command; when this option is not set, the default is to reuse a stage if it is found. --- lib/spack/spack/cmd/install.py | 4 + lib/spack/spack/database.py | 2 + lib/spack/spack/package.py | 44 +++++++- lib/spack/spack/test/install.py | 125 +++++++++++++++++++++ .../repos/builtin.mock/packages/canfail/package.py | 43 +++++++ 5 files changed, 215 insertions(+), 3 deletions(-) create mode 100644 var/spack/repos/builtin.mock/packages/canfail/package.py diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index 87fad76181..f030e5b2df 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -65,6 +65,9 @@ the dependencies""" subparser.add_argument( '--keep-stage', action='store_true', dest='keep_stage', help="don't remove the build stage if installation succeeds") + subparser.add_argument( + '--restage', action='store_true', dest='restage', + help="if a partial install is detected, delete prior state") subparser.add_argument( '-n', '--no-checksum', action='store_true', dest='no_checksum', help="do not check packages against checksum") @@ -307,6 +310,7 @@ def install(parser, args, **kwargs): kwargs.update({ 'keep_prefix': args.keep_prefix, 'keep_stage': args.keep_stage, + 'restage': args.restage, 'install_deps': 'dependencies' in args.things_to_install, 'make_jobs': args.jobs, 'run_tests': args.run_tests, diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index 38ea8d584c..123c3a8998 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -577,6 +577,8 @@ class Database(object): else: # The file doesn't exist, try to traverse the directory. # reindex() takes its own write lock, so no lock here. + with WriteTransaction(self.lock, timeout=_db_lock_timeout): + self._write(None, None, None) self.reindex(spack.store.layout) def _add(self, spec, directory_layout=None, explicit=False): diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 12b91bcdb0..90e55e2ce0 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1188,11 +1188,13 @@ class PackageBase(with_metaclass(PackageMeta, object)): if self.spec.external: return self._process_external_package(explicit) + restage = kwargs.get('restage', False) + partial = self.check_for_unfinished_installation(keep_prefix, restage) + # Ensure package is not already installed layout = spack.store.layout with spack.store.db.prefix_read_lock(self.spec): - if (keep_prefix and os.path.isdir(self.prefix) and - (not self.installed)): + if partial: tty.msg( "Continuing from partial install of %s" % self.name) elif layout.check_installed(self.spec): @@ -1319,7 +1321,8 @@ class PackageBase(with_metaclass(PackageMeta, object)): try: # Create the install prefix and fork the build process. - spack.store.layout.create_install_directory(self.spec) + if not os.path.exists(self.prefix): + spack.store.layout.create_install_directory(self.spec) # Fork a child to do the actual installation spack.build_environment.fork(self, build_process, dirty=dirty) # If we installed then we should keep the prefix @@ -1346,6 +1349,41 @@ class PackageBase(with_metaclass(PackageMeta, object)): if not keep_prefix: self.remove_prefix() + def check_for_unfinished_installation( + self, keep_prefix=False, restage=False): + """Check for leftover files from partially-completed prior install to + prepare for a new install attempt. Options control whether these + files are reused (vs. destroyed). This function considers a package + fully-installed if there is a DB entry for it (in that way, it is + more strict than Package.installed). The return value is used to + indicate when the prefix exists but the install is not complete. + """ + if self.spec.external: + raise ExternalPackageError("Attempted to repair external spec %s" % + self.spec.name) + + with spack.store.db.prefix_write_lock(self.spec): + try: + record = spack.store.db.get_record(self.spec) + installed_in_db = record.installed if record else False + except KeyError: + installed_in_db = False + + partial = False + if not installed_in_db and os.path.isdir(self.prefix): + if not keep_prefix: + self.remove_prefix() + else: + partial = True + + stage_is_managed_in_spack = self.stage.path.startswith( + spack.stage_path) + if restage and stage_is_managed_in_spack: + self.stage.destroy() + self.stage.create() + + return partial + def _do_install_pop_kwargs(self, kwargs): """Pops kwargs from do_install before starting the installation diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py index f10c3a37e9..13d3e2ee94 100644 --- a/lib/spack/spack/test/install.py +++ b/lib/spack/spack/test/install.py @@ -30,6 +30,8 @@ from spack.directory_layout import YamlDirectoryLayout from spack.fetch_strategy import URLFetchStrategy, FetchStrategyComposite from spack.spec import Spec +import os + @pytest.fixture() def install_mockery(tmpdir, config, builtin_mock): @@ -77,6 +79,125 @@ def test_install_and_uninstall(mock_archive): raise +def mock_remove_prefix(*args): + raise MockInstallError( + "Intentional error", + "Mock remove_prefix method intentionally fails") + + +class RemovePrefixChecker(object): + def __init__(self, wrapped_rm_prefix): + self.removed = False + self.wrapped_rm_prefix = wrapped_rm_prefix + + def remove_prefix(self): + self.removed = True + self.wrapped_rm_prefix() + + +class MockStage(object): + def __init__(self, wrapped_stage): + self.wrapped_stage = wrapped_stage + self.test_destroyed = False + + def __enter__(self): + return self + + def __exit__(self, *exc): + return True + + def destroy(self): + self.test_destroyed = True + self.wrapped_stage.destroy() + + def __getattr__(self, attr): + return getattr(self.wrapped_stage, attr) + + +@pytest.mark.usefixtures('install_mockery') +def test_partial_install_delete_prefix_and_stage(mock_archive): + spec = Spec('canfail') + spec.concretize() + pkg = spack.repo.get(spec) + fake_fetchify(mock_archive.url, pkg) + remove_prefix = spack.package.Package.remove_prefix + instance_rm_prefix = pkg.remove_prefix + + try: + spack.package.Package.remove_prefix = mock_remove_prefix + with pytest.raises(MockInstallError): + pkg.do_install() + assert os.path.isdir(pkg.prefix) + rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix) + spack.package.Package.remove_prefix = rm_prefix_checker.remove_prefix + setattr(pkg, 'succeed', True) + pkg.stage = MockStage(pkg.stage) + pkg.do_install(restage=True) + assert rm_prefix_checker.removed + assert pkg.stage.test_destroyed + assert pkg.installed + finally: + spack.package.Package.remove_prefix = remove_prefix + pkg._stage = None + try: + delattr(pkg, 'succeed') + except AttributeError: + pass + + +@pytest.mark.usefixtures('install_mockery') +def test_partial_install_keep_prefix(mock_archive): + spec = Spec('canfail') + spec.concretize() + pkg = spack.repo.get(spec) + # Normally the stage should start unset, but other tests set it + pkg._stage = None + fake_fetchify(mock_archive.url, pkg) + remove_prefix = spack.package.Package.remove_prefix + try: + # If remove_prefix is called at any point in this test, that is an + # error + spack.package.Package.remove_prefix = mock_remove_prefix + with pytest.raises(spack.build_environment.ChildError): + pkg.do_install(keep_prefix=True) + assert os.path.exists(pkg.prefix) + setattr(pkg, 'succeed', True) + pkg.stage = MockStage(pkg.stage) + pkg.do_install(keep_prefix=True) + assert pkg.installed + assert not pkg.stage.test_destroyed + finally: + spack.package.Package.remove_prefix = remove_prefix + pkg._stage = None + try: + delattr(pkg, 'succeed') + except AttributeError: + pass + + +@pytest.mark.usefixtures('install_mockery') +def test_second_install_no_overwrite_first(mock_archive): + spec = Spec('canfail') + spec.concretize() + pkg = spack.repo.get(spec) + fake_fetchify(mock_archive.url, pkg) + remove_prefix = spack.package.Package.remove_prefix + try: + spack.package.Package.remove_prefix = mock_remove_prefix + setattr(pkg, 'succeed', True) + pkg.do_install() + assert pkg.installed + # If Package.install is called after this point, it will fail + delattr(pkg, 'succeed') + pkg.do_install() + finally: + spack.package.Package.remove_prefix = remove_prefix + try: + delattr(pkg, 'succeed') + except AttributeError: + pass + + @pytest.mark.usefixtures('install_mockery') def test_store(mock_archive): spec = Spec('cmake-client').concretized() @@ -102,3 +223,7 @@ def test_failing_build(mock_archive): pkg = spec.package with pytest.raises(spack.build_environment.ChildError): pkg.do_install() + + +class MockInstallError(spack.error.SpackError): + pass diff --git a/var/spack/repos/builtin.mock/packages/canfail/package.py b/var/spack/repos/builtin.mock/packages/canfail/package.py new file mode 100644 index 0000000000..18def38562 --- /dev/null +++ b/var/spack/repos/builtin.mock/packages/canfail/package.py @@ -0,0 +1,43 @@ +############################################################################## +# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC. +# Produced at the Lawrence Livermore National Laboratory. +# +# This file is part of Spack. +# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved. +# LLNL-CODE-647188 +# +# For details, see https://github.com/llnl/spack +# Please also see the LICENSE file for our notice and the LGPL. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License (as +# published by the Free Software Foundation) version 2.1, February 1999. +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and +# conditions of the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +############################################################################## +from spack import * + + +class Canfail(Package): + """Package which fails install unless a special attribute is set""" + + homepage = "http://www.example.com" + url = "http://www.example.com/a-1.0.tar.gz" + + version('1.0', '0123456789abcdef0123456789abcdef') + + def install(self, spec, prefix): + try: + succeed = getattr(self, 'succeed') + if not succeed: + raise InstallError("'succeed' was false") + touch(join_path(prefix, 'an_installation_file')) + except AttributeError: + raise InstallError("'succeed' attribute was not set") -- cgit v1.2.3-70-g09d2