From 650ab563f42d68ea7e9bf0fe296e2b9b1e78755c Mon Sep 17 00:00:00 2001
From: Peter Scheibel <scheibel1@llnl.gov>
Date: Tue, 7 Jul 2020 11:37:36 -0700
Subject: [PATCH] Uninstall: tolerate hook failures when force=true (#16513)

Fixes #16478

This allows an uninstall to proceed even when encountering pre-uninstall
hook failures if the user chooses the --force option for the uninstall.

This also prevents post-uninstall hook failures from raising an exception,
which would terminate a sequence of uninstalls. This isn't likely essential
for #16478, but I think overall it will improve the user experience: if
the post-uninstall hook fails, there isn't much point in terminating a
sequence of spec uninstalls because at the point where the post-uninstall
hook is run, the spec has already been removed from the database (so it
will never have another chance to run).

Notes:

* When doing spack uninstall -a, certain pre/post-uninstall hooks aren't
  important to run, but this isn't easy to track with the current model.
  For example: if you are uninstalling a package and its extension, you
  do not have to do the activation check for the extension.
* This doesn't handle the uninstallation of specs that are not in the DB,
  so it may leave "dangling" specs in the installation prefix
---
 lib/spack/spack/package.py | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 5c4ff23d40..cedcfffed2 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -22,6 +22,7 @@
 import sys
 import textwrap
 import time
+import traceback
 from six import StringIO
 from six import string_types
 from six import with_metaclass
@@ -1744,7 +1745,23 @@ def uninstall_by_spec(spec, force=False, deprecator=None):
         with spack.store.db.prefix_write_lock(spec):
 
             if pkg is not None:
-                spack.hooks.pre_uninstall(spec)
+                try:
+                    spack.hooks.pre_uninstall(spec)
+                except Exception as error:
+                    if force:
+                        error_msg = (
+                            "One or more pre_uninstall hooks have failed"
+                            " for {0}, but Spack is continuing with the"
+                            " uninstall".format(str(spec)))
+                        if isinstance(error, spack.error.SpackError):
+                            error_msg += (
+                                "\n\nError message: {0}".format(str(error)))
+                        tty.warn(error_msg)
+                        # Note that if the uninstall succeeds then we won't be
+                        # seeing this error again and won't have another chance
+                        # to run the hook.
+                    else:
+                        raise
 
             # Uninstalling in Spack only requires removing the prefix.
             if not spec.external:
@@ -1765,7 +1782,20 @@ def uninstall_by_spec(spec, force=False, deprecator=None):
                 spack.store.db.remove(spec)
 
         if pkg is not None:
-            spack.hooks.post_uninstall(spec)
+            try:
+                spack.hooks.post_uninstall(spec)
+            except Exception:
+                # If there is a failure here, this is our only chance to do
+                # something about it: at this point the Spec has been removed
+                # from the DB and prefix, so the post-uninstallation hooks
+                # will not have another chance to run.
+                error_msg = (
+                    "One or more post-uninstallation hooks failed for"
+                    " {0}, but the prefix has been removed (if it is not"
+                    " external).".format(str(spec)))
+                tb_msg = traceback.format_exc()
+                error_msg += "\n\nThe error:\n\n{0}".format(tb_msg)
+                tty.warn(error_msg)
 
         tty.msg("Successfully uninstalled %s" % spec.short_spec)