From 048a0de35bd791f233d0a6f676d841d85e75aae7 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 25 Mar 2022 08:39:09 +0100 Subject: Use the appropriate function to remove files in the stage directory (#29690) We shouldn't be using "remove_linked_tree" to remove the lock file, since that function expects to receive a directory path as an argument. Also, as a further measure to avoid regression, this commit restores the "ignore_errors=True" argument on linux and adds a unit test checking that "remove_linked_tree" doesn't change file permissions as a side effect of a failure to remove. --- lib/spack/llnl/util/filesystem.py | 10 +++++++--- lib/spack/spack/stage.py | 5 ++++- lib/spack/spack/test/llnl/util/filesystem.py | 18 ++++++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index a0129e3550..e16db8ba8a 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -27,7 +27,7 @@ from llnl.util.symlink import symlink from spack.util.executable import Executable from spack.util.path import path_to_os_path, system_path_filter -is_windows = _platform == 'win32' +is_windows = _platform == 'win32' if not is_windows: import grp @@ -1201,12 +1201,16 @@ def remove_linked_tree(path): tty.warn(e) pass + kwargs = {'ignore_errors': True} + if is_windows: + kwargs = {'onerror': onerror} + if os.path.exists(path): if os.path.islink(path): - shutil.rmtree(os.path.realpath(path), onerror=onerror) + shutil.rmtree(os.path.realpath(path), **kwargs) os.unlink(path) else: - shutil.rmtree(path, onerror=onerror) + shutil.rmtree(path, **kwargs) @contextmanager diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index b92c0fbbc8..f289305392 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -823,7 +823,10 @@ def purge(): for stage_dir in os.listdir(root): if stage_dir.startswith(stage_prefix) or stage_dir == '.lock': stage_path = os.path.join(root, stage_dir) - remove_linked_tree(stage_path) + if os.path.isdir(stage_path): + remove_linked_tree(stage_path) + else: + os.remove(stage_path) def get_checksums_for_versions(url_dict, name, **kwargs): diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py index 916d83a352..de3e07d725 100644 --- a/lib/spack/spack/test/llnl/util/filesystem.py +++ b/lib/spack/spack/test/llnl/util/filesystem.py @@ -866,3 +866,21 @@ def test_visit_directory_tree_follow_none(noncyclical_dir_structure): j('b'), ] assert not visitor.symlinked_dirs_after + + +@pytest.mark.regression('29687') +@pytest.mark.parametrize('initial_mode', [ + stat.S_IRUSR | stat.S_IXUSR, + stat.S_IWGRP +]) +@pytest.mark.skipif(sys.platform == 'win32', reason='Windows might change permissions') +def test_remove_linked_tree_doesnt_change_file_permission(tmpdir, initial_mode): + # Here we test that a failed call to remove_linked_tree, due to passing a file + # as an argument instead of a directory, doesn't leave the file with different + # permissions as a side effect of trying to handle the error. + file_instead_of_dir = tmpdir.ensure('foo') + file_instead_of_dir.chmod(initial_mode) + initial_stat = os.stat(str(file_instead_of_dir)) + fs.remove_linked_tree(str(file_instead_of_dir)) + final_stat = os.stat(str(file_instead_of_dir)) + assert final_stat == initial_stat -- cgit v1.2.3-60-g2f50