install --overwrite: use rename instead of tmpdir (#29746)

I tried to use --overwrite on nvhpc, but nvhpc's install size is 16GB. Seems
better to do os.rename in the same directory than moving the directory to
`/tmp`.

- [x] install --overwrite: use rename instead of tmpdir
- [x] use tempfile

4 files changed, 45 insertions, 54 deletions

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index fbc6a3d7ce..53db4bdca9 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -764,39 +764,36 @@ class CouldNotRestoreDirectoryBackup(RuntimeError):
 
 @contextmanager
 @system_path_filter
-def replace_directory_transaction(directory_name, tmp_root=None):
-    """Moves a directory to a temporary space. If the operations executed
-    within the context manager don't raise an exception, the directory is
-    deleted. If there is an exception, the move is undone.
+def replace_directory_transaction(directory_name):
+    """Temporarily renames a directory in the same parent dir. If the operations
+    executed within the context manager don't raise an exception, the renamed directory
+    is deleted. If there is an exception, the move is undone.
 
     Args:
         directory_name (path): absolute path of the directory name
-        tmp_root (path): absolute path of the parent directory where to create
-            the temporary
 
     Returns:
         temporary directory where ``directory_name`` has been moved
     """
     # Check the input is indeed a directory with absolute path.
     # Raise before anything is done to avoid moving the wrong directory
-    assert os.path.isdir(directory_name), \
-        'Invalid directory: ' + directory_name
-    assert os.path.isabs(directory_name), \
-        '"directory_name" must contain an absolute path: ' + directory_name
+    directory_name = os.path.abspath(directory_name)
+    assert os.path.isdir(directory_name), 'Not a directory: ' + directory_name
 
-    directory_basename = os.path.basename(directory_name)
+    # Note: directory_name is normalized here, meaning the trailing slash is dropped,
+    # so dirname is the directory's parent not the directory itself.
+    tmpdir = tempfile.mkdtemp(
+        dir=os.path.dirname(directory_name),
+        prefix='.backup')
 
-    if tmp_root is not None:
-        assert os.path.isabs(tmp_root)
-
-    tmp_dir = tempfile.mkdtemp(dir=tmp_root)
-    tty.debug('Temporary directory created [{0}]'.format(tmp_dir))
-
-    shutil.move(src=directory_name, dst=tmp_dir)
-    tty.debug('Directory moved [src={0}, dest={1}]'.format(directory_name, tmp_dir))
+    # We have to jump through hoops to support Windows, since
+    # os.rename(directory_name, tmpdir) errors there.
+    backup_dir = os.path.join(tmpdir, 'backup')
+    os.rename(directory_name, backup_dir)
+    tty.debug('Directory moved [src={0}, dest={1}]'.format(directory_name, backup_dir))
 
     try:
-        yield tmp_dir
+        yield backup_dir
     except (Exception, KeyboardInterrupt, SystemExit) as inner_exception:
         # Try to recover the original directory, if this fails, raise a
         # composite exception.
@@ -804,10 +801,7 @@ def replace_directory_transaction(directory_name, tmp_root=None):
             # Delete what was there, before copying back the original content
             if os.path.exists(directory_name):
                 shutil.rmtree(directory_name)
-            shutil.move(
-                src=os.path.join(tmp_dir, directory_basename),
-                dst=os.path.dirname(directory_name)
-            )
+            os.rename(backup_dir, directory_name)
         except Exception as outer_exception:
             raise CouldNotRestoreDirectoryBackup(inner_exception, outer_exception)
 
@@ -815,8 +809,8 @@ def replace_directory_transaction(directory_name, tmp_root=None):
         raise
     else:
         # Otherwise delete the temporary directory
-        shutil.rmtree(tmp_dir, ignore_errors=True)
-        tty.debug('Temporary directory deleted [{0}]'.format(tmp_dir))
+        shutil.rmtree(tmpdir, ignore_errors=True)
+        tty.debug('Temporary directory deleted [{0}]'.format(tmpdir))
 
 
 @system_path_filter
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index a8142c8b5f..f99d6baed7 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -2022,11 +2022,10 @@ def build_process(pkg, install_args):
 
 
 class OverwriteInstall(object):
-    def __init__(self, installer, database, task, tmp_root=None):
+    def __init__(self, installer, database, task):
         self.installer = installer
         self.database = database
         self.task = task
-        self.tmp_root = tmp_root
 
     def install(self):
         """
@@ -2036,7 +2035,7 @@ class OverwriteInstall(object):
         install error if installation fails.
         """
         try:
-            with fs.replace_directory_transaction(self.task.pkg.prefix, self.tmp_root):
+            with fs.replace_directory_transaction(self.task.pkg.prefix):
                 self.installer._install_task(self.task)
         except fs.CouldNotRestoreDirectoryBackup as e:
             self.database.remove(self.task.pkg.spec)
diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py
index 4fbafeb278..cd500b1bb8 100644
--- a/lib/spack/spack/test/installer.py
+++ b/lib/spack/spack/test/installer.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import glob
 import os
 import shutil
 import sys
@@ -1175,9 +1176,6 @@ def test_overwrite_install_backup_success(temporary_store, config, mock_packages
     When doing an overwrite install that fails, Spack should restore the backup
     of the original prefix, and leave the original spec marked installed.
     """
-    # Where to store the backups
-    backup = str(tmpdir.mkdir("backup"))
-
     # Get a build task. TODO: refactor this to avoid calling internal methods
     const_arg = installer_args(["b"])
     installer = create_installer(const_arg)
@@ -1202,8 +1200,7 @@ def test_overwrite_install_backup_success(temporary_store, config, mock_packages
 
     fake_installer = InstallerThatWipesThePrefixDir()
     fake_db = FakeDatabase()
-    overwrite_install = inst.OverwriteInstall(
-        fake_installer, fake_db, task, tmp_root=backup)
+    overwrite_install = inst.OverwriteInstall(fake_installer, fake_db, task)
 
     # Installation should throw the installation exception, not the backup
     # failure.
@@ -1223,13 +1220,16 @@ def test_overwrite_install_backup_failure(temporary_store, config, mock_packages
     original prefix. If that fails, the spec is lost, and it should be removed
     from the database.
     """
-    # Where to store the backups
-    backup = str(tmpdir.mkdir("backup"))
-
     class InstallerThatAccidentallyDeletesTheBackupDir:
         def _install_task(self, task):
-            # Remove the backup directory so that restoring goes terribly wrong
-            shutil.rmtree(backup)
+            # 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:
@@ -1250,8 +1250,7 @@ def test_overwrite_install_backup_failure(temporary_store, config, mock_packages
 
     fake_installer = InstallerThatAccidentallyDeletesTheBackupDir()
     fake_db = FakeDatabase()
-    overwrite_install = inst.OverwriteInstall(
-        fake_installer, fake_db, task, tmp_root=backup)
+    overwrite_install = inst.OverwriteInstall(fake_installer, fake_db, task)
 
     # Installation should throw the installation exception, not the backup
     # failure.
diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py
index de3e07d725..c2052223f9 100644
--- a/lib/spack/spack/test/llnl/util/filesystem.py
+++ b/lib/spack/spack/test/llnl/util/filesystem.py
@@ -321,34 +321,33 @@ def test_move_transaction_commit(tmpdir):
     fake_library = tmpdir.mkdir('lib').join('libfoo.so')
     fake_library.write('Just some fake content.')
 
-    old_md5 = fs.hash_directory(str(tmpdir))
-
-    with fs.replace_directory_transaction(str(tmpdir.join('lib'))):
+    with fs.replace_directory_transaction(str(tmpdir.join('lib'))) as backup:
+        assert os.path.isdir(backup)
         fake_library = tmpdir.mkdir('lib').join('libfoo.so')
         fake_library.write('Other content.')
-        new_md5 = fs.hash_directory(str(tmpdir))
 
-    assert old_md5 != fs.hash_directory(str(tmpdir))
-    assert new_md5 == fs.hash_directory(str(tmpdir))
+    assert not os.path.lexists(backup)
+    with open(str(tmpdir.join('lib', 'libfoo.so')), 'r') as f:
+        assert 'Other content.' == f.read()
 
 
 def test_move_transaction_rollback(tmpdir):
 
     fake_library = tmpdir.mkdir('lib').join('libfoo.so')
-    fake_library.write('Just some fake content.')
-
-    h = fs.hash_directory(str(tmpdir))
+    fake_library.write('Initial content.')
 
     try:
-        with fs.replace_directory_transaction(str(tmpdir.join('lib'))):
-            assert h != fs.hash_directory(str(tmpdir))
+        with fs.replace_directory_transaction(str(tmpdir.join('lib'))) as backup:
+            assert os.path.isdir(backup)
             fake_library = tmpdir.mkdir('lib').join('libfoo.so')
-            fake_library.write('Other content.')
+            fake_library.write('New content.')
             raise RuntimeError('')
     except RuntimeError:
         pass
 
-    assert h == fs.hash_directory(str(tmpdir))
+    assert not os.path.lexists(backup)
+    with open(str(tmpdir.join('lib', 'libfoo.so')), 'r') as f:
+        assert 'Initial content.' == f.read()
 
 
 @pytest.mark.regression('10601')