From 94e77333e6c1392a6b95b9a9d8b71cde213ea9c1 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Fri, 5 Jun 2020 00:35:16 -0700 Subject: 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. --- lib/spack/spack/build_environment.py | 32 +++++++++++++++++++++++--------- lib/spack/spack/installer.py | 20 +++++++++++++------- lib/spack/spack/package.py | 11 ++++++----- lib/spack/spack/test/cmd/dev_build.py | 19 +++++++++++++++++++ 4 files changed, 61 insertions(+), 21 deletions(-) (limited to 'lib') 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): -- cgit v1.2.3-70-g09d2