From e7a0b952abc2b0cb5d1253ae77cea018ea792636 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 28 Apr 2022 09:42:42 +0200 Subject: 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 --- lib/spack/llnl/util/filesystem.py | 46 ++++++++++++---------------- lib/spack/spack/installer.py | 5 ++- lib/spack/spack/test/installer.py | 23 +++++++------- lib/spack/spack/test/llnl/util/filesystem.py | 25 ++++++++------- 4 files changed, 45 insertions(+), 54 deletions(-) (limited to 'lib') 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') -- cgit v1.2.3-70-g09d2