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

@@ -65,6 +65,9 @@ def setup_parser(subparser):
     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,

lib/spack/spack/database.py

@@ -577,6 +577,8 @@ def _read(self):
         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):

lib/spack/spack/package.py

@@ -1188,11 +1188,13 @@ def do_install(self,
         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
+            if partial:
-                    (not self.installed)):
                 tty.msg(
                     "Continuing from partial install of %s" % self.name)
             elif layout.check_installed(self.spec):
@@ -1319,6 +1321,7 @@ def build_process(input_stream):
 
         try:
             # Create the install prefix and fork the build process.
+            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)
@@ -1346,6 +1349,41 @@ def build_process(input_stream):
             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
 

lib/spack/spack/test/install.py

@@ -30,6 +30,8 @@
 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

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")