Fix: --overwrite backs up old install dir, but it gets destroyed anyways (#25583)

* Make sure PackageInstaller does not remove the just-restored
  install dir after failure in spack install --overwrite
* Remove cryptic error message and rethrow actual error

3 files changed, 30 insertions, 5 deletions

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index a81425fe96..e44f8c608f 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -692,7 +692,7 @@ def replace_directory_transaction(directory_name, tmp_root=None):
 
     try:
         yield tmp_dir
-    except (Exception, KeyboardInterrupt, SystemExit) as e:
+    except (Exception, KeyboardInterrupt, SystemExit):
         # Delete what was there, before copying back the original content
         if os.path.exists(directory_name):
             shutil.rmtree(directory_name)
@@ -701,10 +701,7 @@ def replace_directory_transaction(directory_name, tmp_root=None):
             dst=os.path.dirname(directory_name)
         )
         tty.debug('DIRECTORY RECOVERED [{0}]'.format(directory_name))
-
-        msg = 'the transactional move of "{0}" failed.'
-        msg += '\n    ' + str(e)
-        raise RuntimeError(msg.format(directory_name))
+        raise
     else:
         # Otherwise delete the temporary directory
         shutil.rmtree(tmp_dir)
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index 8ae2fd410d..eb409caffe 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -1569,6 +1569,9 @@ class PackageInstaller(object):
                             if os.path.exists(rec.path):
                                 with fs.replace_directory_transaction(
                                         rec.path):
+                                    # fs transaction will put the old prefix
+                                    # back on failure, so make sure to keep it.
+                                    keep_prefix = True
                                     self._install_task(task)
                             else:
                                 tty.debug("Missing installation to overwrite")
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index f71c77a35b..67d50d2273 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -126,6 +126,31 @@ def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch):
         pkg.remove_prefix = instance_rm_prefix
 
 
+@pytest.mark.disable_clean_stage_check
+def test_failing_overwrite_install_should_keep_previous_installation(
+    mock_fetch, install_mockery
+):
+    """
+    Make sure that whenever `spack install --overwrite` fails, spack restores
+    the original install prefix instead of cleaning it.
+    """
+    # Do a successful install
+    spec = Spec('canfail').concretized()
+    pkg = spack.repo.get(spec)
+    pkg.succeed = True
+
+    # Do a failing overwrite install
+    pkg.do_install()
+    pkg.succeed = False
+    kwargs = {'overwrite': [spec.dag_hash()]}
+
+    with pytest.raises(Exception):
+        pkg.do_install(**kwargs)
+
+    assert pkg.installed
+    assert os.path.exists(spec.prefix)
+
+
 def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
     dependency = Spec('dependency-install')
     dependency.concretize()