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.

4 files changed, 172 insertions, 3 deletions

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
@@ -66,6 +66,9 @@ the dependencies"""
         '--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")
     subparser.add_argument(
@@ -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