refactor overwrite installs into main installer class

lib/spack/spack/installer.py

@@ -1855,6 +1855,64 @@ def _install_task(self, task: Task, install_status: InstallStatus) -> None:
             self._update_installed(task)
         return rc  # Used by reporters to skip requeued tasks
 
+    def _overwrite_install_task(self, task: Task, install_status: InstallStatus) -> ExecuteResult:
+        """
+        Try to run the install task overwriting the package prefix.
+        If this fails, try to recover the original install prefix. If that fails
+        too, mark the spec as uninstalled. This function always the original
+        install error if installation fails.
+        """
+
+
+class OverwriteInstall:
+    def __init__(
+        self,
+        installer: PackageInstaller,
+        database: spack.database.Database,
+        task: Task,
+        install_status: InstallStatus,
+    ):
+        self.installer = installer
+        self.database = database
+        self.task = task
+        self.install_status = install_status
+
+    def install(self):
+        """
+        Try to run the install task overwriting the package prefix.
+        If this fails, try to recover the original install prefix. If that fails
+        too, mark the spec as uninstalled. This function always the original
+        install error if installation fails.
+        """
+        try:
+            with fs.replace_directory_transaction(task.pkg.prefix):
+                rc = self._install_task(task, install_status)
+                if rc in requeue_results:
+                    raise Requeue  # raise to trigger transactional replacement of directory
+                return rc
+
+        except Requeue:
+            return rc  # This task is requeueing, not failing
+        except fs.CouldNotRestoreDirectoryBackup as e:
+            spack.store.STORE.db.remove(task.pkg.spec)
+            if isinstance(e.inner_exception, Requeue):
+                message_fn = tty.warn
+            else:
+                message_fn = tty.error
+
+            message_fn(
+                f"Recovery of install dir of {task.pkg.name} failed due to "
+                f"{e.outer_exception.__class__.__name__}: {str(e.outer_exception)}. "
+                "The spec is now uninstalled."
+            )
+
+            # Unwrap the actuall installation exception
+            if isinstance(e.inner_exception, Requeue):
+                tty.warn("Task will be requeued to build from source")
+                return rc
+            else:
+                raise e.inner_exception
+
     def _next_is_pri0(self) -> bool:
         """
         Determine if the next task has priority 0
@@ -2253,9 +2311,7 @@ def install(self) -> None:
                 if action == InstallAction.INSTALL:
                     self._install_task(task, install_status)
                 elif action == InstallAction.OVERWRITE:
-                    # spack.store.STORE.db is not really a Database object, but a small
+                    self._overwrite_install_task(task, install_status)
-                    # wrapper -- silence mypy
-                    OverwriteInstall(self, spack.store.STORE.db, task, install_status).install()  # type: ignore[arg-type] # noqa: E501
 
                 # If we installed then we should keep the prefix
                 stop_before_phase = getattr(pkg, "stop_before_phase", None)
@@ -2608,45 +2664,6 @@ def deprecate(spec: "spack.spec.Spec", deprecator: "spack.spec.Spec", link_fn) -
     link_fn(deprecator.prefix, spec.prefix)
 
 
-class OverwriteInstall:
-    def __init__(
-        self,
-        installer: PackageInstaller,
-        database: spack.database.Database,
-        task: Task,
-        install_status: InstallStatus,
-    ):
-        self.installer = installer
-        self.database = database
-        self.task = task
-        self.install_status = install_status
-
-    def install(self):
-        """
-        Try to run the install task overwriting the package prefix.
-        If this fails, try to recover the original install prefix. If that fails
-        too, mark the spec as uninstalled. This function always the original
-        install error if installation fails.
-        """
-        try:
-            with fs.replace_directory_transaction(self.task.pkg.prefix):
-                rc = self.installer._install_task(self.task, self.install_status)
-                if rc in requeue_results:
-                    raise Requeue  # raise to trigger transactional replacement of directory
-        except Requeue:
-            pass  # this job is requeuing, not failing
-        except fs.CouldNotRestoreDirectoryBackup as e:
-            self.database.remove(self.task.pkg.spec)
-            tty.error(
-                f"Recovery of install dir of {self.task.pkg.name} failed due to "
-                f"{e.outer_exception.__class__.__name__}: {str(e.outer_exception)}. "
-                "The spec is now uninstalled."
-            )
-
-            # Unwrap the actual installation exception.
-            raise e.inner_exception
-
-
 class Requeue(Exception):
     """Raised when we need an error to indicate a requeueing situation.
 

lib/spack/spack/test/installer.py

@@ -1141,7 +1141,7 @@ def test_install_implicit(install_mockery, mock_fetch):
     assert not create_build_task(pkg).explicit
 
 
-def test_overwrite_install_backup_success(temporary_store, config, mock_packages, tmpdir):
+def test_overwrite_install_backup_success(temporary_store, config, mock_packages, monkeypatch):
     """
     When doing an overwrite install that fails, Spack should restore the backup
     of the original prefix, and leave the original spec marked installed.
@@ -1156,11 +1156,12 @@ def test_overwrite_install_backup_success(temporary_store, config, mock_packages
     installed_file = os.path.join(task.pkg.prefix, "some_file")
     fs.touchp(installed_file)
 
-    class InstallerThatWipesThePrefixDir:
+    def _install_task(self, task, install_status):
-        def _install_task(self, task, install_status):
+        shutil.rmtree(task.pkg.prefix, ignore_errors=True)
-            shutil.rmtree(task.pkg.prefix, ignore_errors=True)
+        fs.mkdirp(task.pkg.prefix)
-            fs.mkdirp(task.pkg.prefix)
+        raise Exception("Some fatal install error")
-            raise Exception("Some fatal install error")
+
+    monkeypatch.setattr(inst.PackageInstaller, "_install_task", _install_task)
 
     class FakeDatabase:
         called = False
@@ -1168,46 +1169,25 @@ class FakeDatabase:
         def remove(self, spec):
             self.called = True
 
-    fake_installer = InstallerThatWipesThePrefixDir()
+    monkeypatch.setattr(spack.store.STORE, "db", FakeDatabase())
-    fake_db = FakeDatabase()
-    overwrite_install = inst.OverwriteInstall(fake_installer, fake_db, task, None)
 
     # Installation should throw the installation exception, not the backup
     # failure.
     with pytest.raises(Exception, match="Some fatal install error"):
-        overwrite_install.install()
+        installer._overwrite_install_task(task, None)
 
     # Make sure the package is not marked uninstalled and the original dir
     # is back.
-    assert not fake_db.called
+    assert not spack.store.STORE.db.called
     assert os.path.exists(installed_file)
 
 
-def test_overwrite_install_backup_failure(temporary_store, config, mock_packages, tmpdir):
+def test_overwrite_install_backup_failure(temporary_store, config, mock_packages, monkeypatch):
     """
     When doing an overwrite install that fails, Spack should try to recover the
     original prefix. If that fails, the spec is lost, and it should be removed
     from the database.
     """
-    # Note: this test relies on installing a package with no dependencies
-
-    class InstallerThatAccidentallyDeletesTheBackupDir:
-        def _install_task(self, task, install_status):
-            # Remove the backup directory, which is at the same level as the prefix,
-            # starting with .backup
-            backup_glob = os.path.join(
-                os.path.dirname(os.path.normpath(task.pkg.prefix)), ".backup*"
-            )
-            for backup in glob.iglob(backup_glob):
-                shutil.rmtree(backup)
-            raise Exception("Some fatal install error")
-
-    class FakeDatabase:
-        called = False
-
-        def remove(self, spec):
-            self.called = True
-
     # Get a build task. TODO: refactor this to avoid calling internal methods
     installer = create_installer(["pkg-c"])
     installer._init_queue()
@@ -1217,18 +1197,32 @@ def remove(self, spec):
     installed_file = os.path.join(task.pkg.prefix, "some_file")
     fs.touchp(installed_file)
 
-    fake_installer = InstallerThatAccidentallyDeletesTheBackupDir()
+    def _install_task(self, task, install_status):
-    fake_db = FakeDatabase()
+        # Remove the backup directory, which is at the same level as the prefix,
-    overwrite_install = inst.OverwriteInstall(fake_installer, fake_db, task, None)
+        # starting with .backup
+        backup_glob = os.path.join(os.path.dirname(os.path.normpath(task.pkg.prefix)), ".backup*")
+        for backup in glob.iglob(backup_glob):
+            shutil.rmtree(backup)
+        raise Exception("Some fatal install error")
+
+    monkeypatch.setattr(inst.PackageInstaller, "_install_task", _install_task)
+
+    class FakeDatabase:
+        called = False
+
+        def remove(self, spec):
+            self.called = True
+
+    monkeypatch.setattr(spack.store.STORE, "db", FakeDatabase())
 
     # Installation should throw the installation exception, not the backup
     # failure.
     with pytest.raises(Exception, match="Some fatal install error"):
-        overwrite_install.install()
+        installer._overwrite_install_task(task, None)
 
     # Make sure that `remove` was called on the database after an unsuccessful
     # attempt to restore the backup.
-    assert fake_db.called
+    assert spack.store.STORE.db.called
 
 
 def test_term_status_line():