spack dev-build: Do not mark -u builds in database (#16333)

Builds can be stopped before the final install phase due to user requests. Those builds 
should not be registered as installed in the database.

We had code intended to handle this but:

  1. It caught the wrong type of exception
  2. We were catching these exceptions to suppress them at a lower level in the stack

This PR allows the StopIteration to propagate through a ChildError, and catches it
properly. Also added to an existing test to prevent regression.

4 files changed, 61 insertions, 21 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 8beedf6c97..f57ecd1e58 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -812,12 +812,11 @@ def fork(pkg, function, dirty, fake):
                 setup_package(pkg, dirty=dirty)
             return_value = function()
             child_pipe.send(return_value)
-        except StopIteration as e:
-            # StopIteration is used to stop installations
-            # before the final stage, mainly for debug purposes
-            tty.msg(e)
-            child_pipe.send(None)
 
+        except StopPhase as e:
+            # Do not create a full ChildError from this, it's not an error
+            # it's a control statement.
+            child_pipe.send(e)
         except BaseException:
             # catch ANYTHING that goes wrong in the child process
             exc_type, exc, tb = sys.exc_info()
@@ -869,15 +868,20 @@ def fork(pkg, function, dirty, fake):
     child_result = parent_pipe.recv()
     p.join()
 
+    # If returns a StopPhase, raise it
+    if isinstance(child_result, StopPhase):
+        # do not print
+        raise child_result
+
     # let the caller know which package went wrong.
     if isinstance(child_result, InstallError):
         child_result.pkg = pkg
 
-    # If the child process raised an error, print its output here rather
-    # than waiting until the call to SpackError.die() in main(). This
-    # allows exception handling output to be logged from within Spack.
-    # see spack.main.SpackCommand.
     if isinstance(child_result, ChildError):
+        # If the child process raised an error, print its output here rather
+        # than waiting until the call to SpackError.die() in main(). This
+        # allows exception handling output to be logged from within Spack.
+        # see spack.main.SpackCommand.
         child_result.print_context()
         raise child_result
 
@@ -1066,3 +1070,13 @@ class ChildError(InstallError):
 def _make_child_error(msg, module, name, traceback, build_log, context):
     """Used by __reduce__ in ChildError to reconstruct pickled errors."""
     return ChildError(msg, module, name, traceback, build_log, context)
+
+
+class StopPhase(spack.error.SpackError):
+    """Pickle-able exception to control stopped builds."""
+    def __reduce__(self):
+        return _make_stop_phase, (self.message, self.long_message)
+
+
+def _make_stop_phase(msg, long_msg):
+    return StopPhase(msg, long_msg)
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index 8886f3ed06..0eeec020bc 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -784,6 +784,9 @@ class PackageInstaller(object):
         The ``stop_before`` or ``stop_at`` arguments are removed from the
         installation arguments.
 
+        The last phase is also set to None if it is the last phase of the
+        package already
+
         Args:
             kwargs:
               ``stop_before``': stop before execution of this phase (or None)
@@ -800,6 +803,10 @@ class PackageInstaller(object):
                 self.pkg.last_phase not in self.pkg.phases:
             tty.die('\'{0}\' is not an allowed phase for package {1}'
                     .format(self.pkg.last_phase, self.pkg.name))
+        # If we got a last_phase, make sure it's not already last
+        if self.pkg.last_phase and \
+                self.pkg.last_phase == self.pkg.phases[-1]:
+            self.pkg.last_phase = None
 
     def _cleanup_all_tasks(self):
         """Cleanup all build tasks to include releasing their locks."""
@@ -1164,13 +1171,12 @@ class PackageInstaller(object):
             if task.compiler:
                 spack.compilers.add_compilers_to_config(
                     spack.compilers.find_compilers([pkg.spec.prefix]))
-
-        except StopIteration as e:
-            # A StopIteration exception means that do_install was asked to
-            # stop early from clients.
-            tty.msg('{0} {1}'.format(self.pid, str(e)))
-            tty.msg('Package stage directory : {0}'
-                    .format(pkg.stage.source_path))
+        except spack.build_environment.StopPhase as e:
+            # A StopPhase exception means that do_install was asked to
+            # stop early from clients, and is not an error at this point
+            tty.debug('{0} {1}'.format(self.pid, str(e)))
+            tty.debug('Package stage directory : {0}'
+                      .format(pkg.stage.source_path))
 
     _install_task.__doc__ += install_args_docstring
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index c9774c5b29..dc32effbec 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -116,16 +116,17 @@ class InstallPhase(object):
 
     def _on_phase_start(self, instance):
         # If a phase has a matching stop_before_phase attribute,
-        # stop the installation process raising a StopIteration
+        # stop the installation process raising a StopPhase
         if getattr(instance, 'stop_before_phase', None) == self.name:
-            raise StopIteration('Stopping before \'{0}\' phase'
-                                .format(self.name))
+            from spack.build_environment import StopPhase
+            raise StopPhase('Stopping before \'{0}\' phase'.format(self.name))
 
     def _on_phase_exit(self, instance):
         # If a phase has a matching last_phase attribute,
-        # stop the installation process raising a StopIteration
+        # stop the installation process raising a StopPhase
         if getattr(instance, 'last_phase', None) == self.name:
-            raise StopIteration('Stopping at \'{0}\' phase'.format(self.name))
+            from spack.build_environment import StopPhase
+            raise StopPhase('Stopping at \'{0}\' phase'.format(self.name))
 
     def copy(self):
         try:
diff --git a/lib/spack/spack/test/cmd/dev_build.py b/lib/spack/spack/test/cmd/dev_build.py
index 37c40e787d..271faa845d 100644
--- a/lib/spack/spack/test/cmd/dev_build.py
+++ b/lib/spack/spack/test/cmd/dev_build.py
@@ -54,6 +54,25 @@ def test_dev_build_until(tmpdir, mock_packages, install_mockery):
             assert f.read() == spec.package.replacement_string
 
     assert not os.path.exists(spec.prefix)
+    assert not spack.store.db.query(spec, installed=True)
+
+
+def test_dev_build_until_last_phase(tmpdir, mock_packages, install_mockery):
+    # Test that we ignore the last_phase argument if it is already last
+    spec = spack.spec.Spec('dev-build-test-install@0.0.0').concretized()
+
+    with tmpdir.as_cwd():
+        with open(spec.package.filename, 'w') as f:
+            f.write(spec.package.original_string)
+
+        dev_build('-u', 'install', 'dev-build-test-install@0.0.0')
+
+        assert spec.package.filename in os.listdir(os.getcwd())
+        with open(spec.package.filename, 'r') as f:
+            assert f.read() == spec.package.replacement_string
+
+    assert os.path.exists(spec.prefix)
+    assert spack.store.db.query(spec, installed=True)
 
 
 def test_dev_build_before_until(tmpdir, mock_packages, install_mockery):