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.

3 files changed, 29 insertions, 4 deletions

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