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

lib/spack/spack/cmd/install.py

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

lib/spack/spack/directory_layout.py

@@ -239,6 +239,17 @@ def build_packages_path(self, spec):
         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 @@ def create_install_directory(self, spec):
     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)

lib/spack/spack/package.py

@@ -856,7 +856,7 @@ def provides(self, vpkg_name):
 
     @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 @@ def do_install(self,
                     (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 @@ def build_process(input_stream):
 
         try:
             # Create the install prefix and fork the build process.
+            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 @@ def build_process(input_stream):
             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
 

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,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

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

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