Windows permissions: uninstalling and cleaning stages (#29714)

When running on Windows, Spack may generate files in the stage/install
prefixes that do not have write permissions, which prevents the
removal of those directories (e.g. when cleaning stages or uninstalling).
There should be a refactoring to avoid this in the first place, but that
is assumed to be longer term, so the temporary fix is to make such files
writable if they are not. This PR:

* Automatically handles these permissions errors when uninstalling
  packages from the Spack root (makes then writable)
* Updates similar already-existing logic when removing Spack-managed
  stage directories (the error-handling was assuming all errors were
  permissions errors and was therefore handling other errors
  inappropriately)

Note: these permissions issues only appear on Windows so this logic is
only applied there (permissions are not modified for this purpose on
Linux etc.).

This also adds special handling for a case where calling `isdir`
on an `os.DirEntry` object would fail for improperly-created symlinks
(e.g. on Windows, using `os.symlink` without `target_is_directory=True`).
Note this specific issue only came up when enabling link_tree tests
(specifically `source_merge_visitor_cant_be_cyclical`).

3 files changed, 74 insertions, 20 deletions

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index 973d2f9de3..5c9b225c72 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -1095,7 +1095,32 @@ def visit_directory_tree(root, visitor, rel_path='', depth=0):
     for f in dir_entries:
         if sys.version_info >= (3, 5, 0):
             rel_child = os.path.join(rel_path, f.name)
-            islink, isdir = f.is_symlink(), f.is_dir()
+            islink = f.is_symlink()
+            # On Windows, symlinks to directories are distinct from
+            # symlinks to files, and it is possible to create a
+            # broken symlink to a directory (e.g. using os.symlink
+            # without `target_is_directory=True`), invoking `isdir`
+            # on a symlink on Windows that is broken in this manner
+            # will result in an error. In this case we can work around
+            # the issue by reading the target and resolving the
+            # directory ourselves
+            try:
+                isdir = f.is_dir()
+            except OSError as e:
+                if is_windows and hasattr(e, 'winerror')\
+                   and e.winerror == 5 and islink:
+                    # if path is a symlink, determine destination and
+                    # evaluate file vs directory
+                    link_target = resolve_link_target_relative_to_the_link(f)
+                    # link_target might be relative but
+                    # resolve_link_target_relative_to_the_link
+                    # will ensure that if so, that it is relative
+                    # to the CWD and therefore
+                    # makes sense
+                    isdir = os.path.isdir(link_target)
+                else:
+                    raise e
+
         else:
             rel_child = os.path.join(rel_path, f)
             lexists, islink, isdir = lexists_islink_isdir(os.path.join(dir, f))
@@ -1103,7 +1128,7 @@ def visit_directory_tree(root, visitor, rel_path='', depth=0):
                 continue
 
         if not isdir:
-            # Handle files
+            # handle files
             visitor.visit_file(root, rel_child, depth)
         elif not islink and visitor.before_visit_dir(root, rel_child, depth):
             # Handle ordinary directories
@@ -1178,6 +1203,35 @@ def remove_if_dead_link(path):
         os.unlink(path)
 
 
+def readonly_file_handler(ignore_errors=False):
+    # TODO: generate stages etc. with write permissions wherever
+    # so this callback is no-longer required
+    """
+    Generate callback for shutil.rmtree to handle permissions errors on
+    Windows. Some files may unexpectedly lack write permissions even
+    though they were generated by Spack on behalf of the user (e.g. the
+    stage), so this callback will detect such cases and modify the
+    permissions if that is the issue. For other errors, the fallback
+    is either to raise (if ignore_errors is False) or ignore (if
+    ignore_errors is True). This is only intended for Windows systems
+    and will raise a separate error if it is ever invoked (by accident)
+    on a non-Windows system.
+    """
+    def error_remove_readonly(func, path, exc):
+        if not is_windows:
+            raise RuntimeError("This method should only be invoked on Windows")
+        excvalue = exc[1]
+        if is_windows and func in (os.rmdir, os.remove, os.unlink) and\
+           excvalue.errno == errno.EACCES:
+            # change the file to be readable,writable,executable: 0777
+            os.chmod(path, stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)
+            # retry
+            func(path)
+        elif not ignore_errors:
+            raise
+    return error_remove_readonly
+
+
 @system_path_filter
 def remove_linked_tree(path):
     """Removes a directory and its contents.
@@ -1185,23 +1239,18 @@ def remove_linked_tree(path):
     If the directory is a symlink, follows the link and removes the real
     directory before removing the link.
 
+    This method will force-delete files on Windows
+
     Parameters:
         path (str): Directory to be removed
     """
-    # On windows, cleaning a Git stage can be an issue
-    # as git leaves readonly files that Python handles
-    # poorly on Windows. Remove readonly status and try again
-    def onerror(func, path, exe_info):
-        os.chmod(path, stat.S_IWUSR)
-        try:
-            func(path)
-        except Exception as e:
-            tty.warn(e)
-            pass
-
     kwargs = {'ignore_errors': True}
+
+    # Windows readonly files cannot be removed by Python
+    # directly.
     if is_windows:
-        kwargs = {'onerror': onerror}
+        kwargs['ignore_errors'] = False
+        kwargs['onerror'] = readonly_file_handler(ignore_errors=True)
 
     if os.path.exists(path):
         if os.path.islink(path):
diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py
index a33eff045a..33d3832a4c 100644
--- a/lib/spack/spack/directory_layout.py
+++ b/lib/spack/spack/directory_layout.py
@@ -9,6 +9,7 @@ import os
 import posixpath
 import re
 import shutil
+import sys
 import tempfile
 from contextlib import contextmanager
 
@@ -24,6 +25,7 @@ import spack.spec
 import spack.util.spack_json as sjson
 from spack.error import SpackError
 
+is_windows = sys.platform == 'win32'
 # Note: Posixpath is used here as opposed to
 # os.path.join due to spack.spec.Spec.format
 # requiring forward slash path seperators at this stage
@@ -349,6 +351,14 @@ class DirectoryLayout(object):
         path = self.path_for_spec(spec)
         assert(path.startswith(self.root))
 
+        # Windows readonly files cannot be removed by Python
+        # directly, change permissions before attempting to remove
+        if is_windows:
+            kwargs = {'ignore_errors': False,
+                      'onerror': fs.readonly_file_handler(ignore_errors=False)}
+        else:
+            kwargs = {}  # the default value for ignore_errors is false
+
         if deprecated:
             if os.path.exists(path):
                 try:
@@ -357,10 +367,9 @@ class DirectoryLayout(object):
                     os.remove(metapath)
                 except OSError as e:
                     raise six.raise_from(RemoveFailedError(spec, path, e), e)
-
         elif os.path.exists(path):
             try:
-                shutil.rmtree(path)
+                shutil.rmtree(path, **kwargs)
             except OSError as e:
                 raise six.raise_from(RemoveFailedError(spec, path, e), e)
 
diff --git a/lib/spack/spack/test/llnl/util/link_tree.py b/lib/spack/spack/test/llnl/util/link_tree.py
index 296a2cd263..3b66c376b2 100644
--- a/lib/spack/spack/test/llnl/util/link_tree.py
+++ b/lib/spack/spack/test/llnl/util/link_tree.py
@@ -4,7 +4,6 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import os
-import sys
 
 import pytest
 
@@ -14,9 +13,6 @@ from llnl.util.symlink import islink
 
 from spack.stage import Stage
 
-pytestmark = pytest.mark.skipif(sys.platform == "win32",
-                                reason="does not run on windows")
-
 
 @pytest.fixture()
 def stage():