Override partial installs by default (#3530)

* Package install remove prior unfinished installs

Depending on how spack is terminated in the middle of building a
package it may leave a partially installed package in the install
prefix. Originally Spack treated the package as being installed if
the prefix was present, in which case the user would have to
manually remove the installation prefix before restarting an
install. This commit adds a more thorough check to ensure that a
package is actually installed. If the installation prefix is present
but Spack determines that the install did not complete, it removes
the installation prefix and starts a new install; if the user has
enabled --keep-prefix, then Spack reverts to its old behavior.

* Added test for partial install handling

* Added test for restoring DB

* Style fixes

* Restoring 2.6 compatibility

* Relocated repair logic to separate function

* If --keep-prefix is set, package installs will continue an install from an existing prefix if one is present

* check metadata consistency when continuing partial install

* Added --force option to make spack reinstall a package (and all dependencies) from scratch

* Updated bash completion; removed '-f' shorthand for '--force' for install command

* dont use multiple write modes for completion file

4 files changed, 203 insertions, 7 deletions

diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index fb01fc2d5e..08dc080e8e 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -72,6 +72,9 @@ the dependencies"""
     subparser.add_argument(
         '--fake', action='store_true', dest='fake',
         help="fake install. just remove prefix and create a fake file")
+    subparser.add_argument(
+        '--force', action='store_true', dest='force',
+        help='Install again even if package is already installed.')
 
     cd_group = subparser.add_mutually_exclusive_group()
     arguments.add_common_arguments(cd_group, ['clean', 'dirty'])
@@ -319,6 +322,12 @@ def install(parser, args, **kwargs):
         tty.error('The `spack install` command requires a spec to install.')
 
     for spec in specs:
+        if args.force:
+            for s in spec.traverse():
+                if s.package.installed:
+                    tty.msg("Clearing %s for new installation" % s.name)
+                    s.package.do_uninstall(force=True)
+
         # Check if we were asked to produce some log for dashboards
         if args.log_format is not None:
             # Compute the filename for logging
diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py
index 9d09875484..e220a2d430 100644
--- a/lib/spack/spack/directory_layout.py
+++ b/lib/spack/spack/directory_layout.py
@@ -239,6 +239,17 @@ class YamlDirectoryLayout(DirectoryLayout):
         return join_path(self.path_for_spec(spec), self.metadata_dir,
                          self.packages_dir)
 
+    def _completion_marker_file(self, spec):
+        return join_path(self.path_for_spec(spec), self.metadata_dir,
+                         'complete')
+
+    def mark_complete(self, spec):
+        with open(self._completion_marker_file(spec), 'w'):
+            pass
+
+    def completed_install(self, spec):
+        return os.path.exists(self._completion_marker_file(spec))
+
     def create_install_directory(self, spec):
         _check_concrete(spec)
 
@@ -252,26 +263,34 @@ class YamlDirectoryLayout(DirectoryLayout):
     def check_installed(self, spec):
         _check_concrete(spec)
         path = self.path_for_spec(spec)
-        spec_file_path = self.spec_file_path(spec)
 
         if not os.path.isdir(path):
             return None
+        elif not self.completed_install(spec):
+            raise InconsistentInstallDirectoryError(
+                'The prefix %s contains a partial install' % path)
+
+        self.check_metadata_consistency(spec)
+        return path
+
+    def check_metadata_consistency(self, spec):
+        spec_file_path = self.spec_file_path(spec)
 
         if not os.path.isfile(spec_file_path):
             raise InconsistentInstallDirectoryError(
                 'Install prefix exists but contains no spec.yaml:',
-                "  " + path)
+                "  " + spec.prefix)
 
         installed_spec = self.read_spec(spec_file_path)
         if installed_spec == spec:
-            return path
+            return
 
         # DAG hashes currently do not include build dependencies.
         #
         # TODO: remove this when we do better concretization and don't
         # ignore build-only deps in hashes.
         elif installed_spec == spec.copy(deps=('link', 'run')):
-            return path
+            return
 
         if spec.dag_hash() == installed_spec.dag_hash():
             raise SpecHashCollisionError(spec, installed_spec)
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 108ddeff07..e6eea35a80 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -856,7 +856,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
 
     @property
     def installed(self):
-        return os.path.isdir(self.prefix)
+        return spack.store.layout.completed_install(self.spec)
 
     @property
     def prefix_lock(self):
@@ -1174,10 +1174,16 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                     (self.name, self.spec.external))
             return
 
+        self.repair_partial(keep_prefix)
+
         # Ensure package is not already installed
         layout = spack.store.layout
         with self._prefix_read_lock():
-            if layout.check_installed(self.spec):
+            if (keep_prefix and os.path.isdir(self.prefix) and 
+                    (not self.installed)):
+                tty.msg(
+                    "Continuing from partial install of %s" % self.name)
+            elif layout.check_installed(self.spec):
                 tty.msg(
                     "%s is already installed in %s" % (self.name, self.prefix))
                 rec = spack.store.db.get_record(self.spec)
@@ -1305,9 +1311,11 @@ 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 keep_prefix) or (not os.path.isdir(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)
+            spack.store.layout.mark_complete(self.spec)
             # If we installed then we should keep the prefix
             keep_prefix = True if self.last_phase is None else keep_prefix
             # note: PARENT of the build process adds the new package to
@@ -1332,6 +1340,43 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             if not keep_prefix:
                 self.remove_prefix()
 
+    def repair_partial(self, continue_with_partial=False):
+        """If continue_with_partial is not set, this ensures that the package
+           is either fully-installed or that the prefix is removed. If the
+           package is installed but there is no DB entry then this adds a
+           record. If continue_with_partial is not set this also clears the
+           stage directory to start an installation from scratch.
+        """
+        layout = spack.store.layout
+        with self._prefix_read_lock():
+            if (os.path.isdir(self.prefix) and not self.installed and
+                    not continue_with_partial):
+                spack.hooks.pre_uninstall(self)
+                self.remove_prefix()
+                try:
+                    spack.store.db.remove(self.spec)
+                except KeyError:
+                    pass
+                spack.hooks.post_uninstall(self)
+                tty.msg("Removed partial install for %s" %
+                        self.spec.short_spec)
+            elif self.installed and layout.check_installed(self.spec):
+                try:
+                    spack.store.db.get_record(self.spec)
+                except KeyError:
+                    tty.msg("Repairing db for %s" % self.name)
+                    spack.store.db.add(self.spec)
+
+            if continue_with_partial and not self.installed:
+                try:
+                    layout.check_metadata_consistency(self.spec)
+                except directory_layout.DirectoryLayoutError:
+                    self.remove_prefix()
+
+        if not continue_with_partial:
+            self.stage.destroy()
+            self.stage.create()
+
     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..08694f7ee8 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,123 @@ def test_install_and_uninstall(mock_archive):
         raise
 
 
+def mock_remove_prefix(*args):
+    raise MockInstallError(
+        "Intentional error",
+        "Mock remove_prefix method intentionally fails")
+
+
+@pytest.mark.usefixtures('install_mockery')
+def test_partial_install(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
+        try:
+            pkg.do_install()
+        except MockInstallError:
+            pass
+        spack.package.Package.remove_prefix = remove_prefix
+        setattr(pkg, 'succeed', True)
+        pkg.do_install()
+        assert pkg.installed
+    finally:
+        spack.package.Package.remove_prefix = remove_prefix
+        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)
+    fake_fetchify(mock_archive.url, pkg)
+    remove_prefix = spack.package.Package.remove_prefix
+    try:
+        spack.package.Package.remove_prefix = mock_remove_prefix
+        try:
+            pkg.do_install()
+        except MockInstallError:
+            pass
+        # Don't repair remove_prefix at this point, set keep_prefix so that
+        # Spack continues with a partial install
+        setattr(pkg, 'succeed', True)
+        pkg.do_install(keep_prefix=True)
+        assert pkg.installed
+    finally:
+        spack.package.Package.remove_prefix = remove_prefix
+        try:
+            delattr(pkg, 'succeed')
+        except AttributeError:
+            pass
+
+
+@pytest.mark.usefixtures('install_mockery')
+def test_partial_install_keep_prefix_check_metadata(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
+        try:
+            pkg.do_install()
+        except MockInstallError:
+            pass
+        os.remove(spack.store.layout.spec_file_path(spec))
+        spack.package.Package.remove_prefix = remove_prefix
+        setattr(pkg, 'succeed', True)
+        pkg.do_install(keep_prefix=True)
+        assert pkg.installed
+        spack.store.layout.check_metadata_consistency(spec)
+    finally:
+        spack.package.Package.remove_prefix = remove_prefix
+        try:
+            delattr(pkg, 'succeed')
+        except AttributeError:
+            pass
+
+
+@pytest.mark.usefixtures('install_mockery')
+def test_install_succeeds_but_db_add_fails(mock_archive):
+    """If an installation succeeds but the database is not updated, make sure
+       that the database is updated for a future install."""
+    spec = Spec('cmake')
+    spec.concretize()
+    pkg = spack.repo.get(spec)
+    fake_fetchify(mock_archive.url, pkg)
+    remove_prefix = spack.package.Package.remove_prefix
+    db_add = spack.store.db.add
+
+    def mock_db_add(*args, **kwargs):
+        raise MockInstallError(
+            "Intentional error", "Mock db add method intentionally fails")
+
+    try:
+        spack.package.Package.remove_prefix = mock_remove_prefix
+        spack.store.db.add = mock_db_add
+        try:
+            pkg.do_install()
+        except MockInstallError:
+            pass
+        assert pkg.installed
+
+        spack.package.Package.remove_prefix = remove_prefix
+        spack.store.db.add = db_add
+        pkg.do_install()
+        assert spack.store.db.get_record(spec)
+    except:
+        spack.package.Package.remove_prefix = remove_prefix
+        raise
+
+
 @pytest.mark.usefixtures('install_mockery')
 def test_store(mock_archive):
     spec = Spec('cmake-client').concretized()
@@ -102,3 +221,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