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

6 files changed, 245 insertions, 8 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
diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index 819dcc06ab..1167690fa2 100755
--- a/share/spack/spack-completion.bash
+++ b/share/spack/spack-completion.bash
@@ -407,7 +407,7 @@ function _spack_install {
     then
         compgen -W "-h --help --only -j --jobs --keep-prefix --keep-stage
                     -n --no-checksum -v --verbose --fake --clean --dirty
-                    --run-tests --log-format --log-file" -- "$cur"
+                    --run-tests --log-format --log-file --force" -- "$cur"
     else
         compgen -W "$(_all_packages)" -- "$cur"
     fi
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..bef4f6e838
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/canfail/package.py
@@ -0,0 +1,41 @@
+##############################################################################
+# 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:
+            getattr(self, 'succeed')
+            touch(join_path(prefix, 'an_installation_file'))
+        except AttributeError:
+            raise InstallError()