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
This commit is contained in:
parent
8b85b33ba5
commit
e7a0b952ab
@ -764,39 +764,36 @@ def __init__(self, inner_exception, outer_exception):
|
||||
|
||||
@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
|
||||
|
@ -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 @@ def install(self):
|
||||
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)
|
||||
|
@ -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 remove(self, spec):
|
||||
|
||||
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 remove(self, spec):
|
||||
|
||||
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.
|
||||
|
@ -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')
|
||||
|
Loading…
Reference in New Issue
Block a user