Fix temporary directory removal on Windows (copier-org#358)

* tests: Add temporary directory deletion tests

* ci: Run tests on Windows for Python 3.6 and 3.7 again

* fix: Implement more robust temporary directory deletion

* docs: Document temporary directory class and error handler

* chore: Update poetry.lock

* style: Format code

Co-authored-by: Timothée Mazzucotelli <timothee.mazzucotelli@externe.e-i.com>

.github/workflows/ci.yml

@@ -34,10 +34,6 @@ jobs:
           - ubuntu-latest
           - windows-latest
         python-version: [3.6, 3.7, 3.8, 3.9]
-        exclude:
-          # Windows is only supported on Python 3.8+
-          - {os: windows-latest, python-version: 3.6}
-          - {os: windows-latest, python-version: 3.7}
     runs-on: ${{ matrix.os }}
     steps:
       # HACK https://github.com/actions/cache/issues/315

copier/main.py

@@ -4,7 +4,6 @@
 import platform
 import subprocess
 import sys
-import tempfile
 from contextlib import suppress
 from dataclasses import asdict, field, replace
 from functools import partial
@@ -29,7 +28,7 @@
 from .errors import UserMessageError
 from .subproject import Subproject
 from .template import Template
-from .tools import Style, printf, to_nice_yaml
+from .tools import Style, TemporaryDirectory, printf, to_nice_yaml
 from .types import (
     AnyByStrDict,
     JSONSerializable,
@@ -613,7 +612,7 @@ def run_update(self) -> None:
                 "Downgrades are not supported."
             )
         # Copy old template into a temporary destination
-        with tempfile.TemporaryDirectory(prefix=f"{__name__}.update_diff.") as dst_temp:
+        with TemporaryDirectory(prefix=f"{__name__}.update_diff.") as dst_temp:
             old_worker = replace(
                 self,
                 dst_path=dst_temp,

copier/tools.py

@@ -1,17 +1,22 @@
 """Some utility functions."""
 
+import errno
+import os
 import shutil
+import stat
 import sys
+import tempfile
+import warnings
 from contextlib import suppress
 from pathlib import Path
-from typing import Any, Optional, TextIO, Union
+from typing import Any, Callable, Optional, TextIO, Union
 
 import colorama
 from packaging.version import Version
 from pydantic import StrictBool
 from yaml import safe_dump
 
-from .types import IntSeq
+from .types import ExcInfo, IntSeq
 
 try:
     from importlib.metadata import version
@@ -135,3 +140,46 @@ def force_str_end(original_str: str, end: str = "\n") -> str:
     if not original_str.endswith(end):
         return original_str + end
     return original_str
+
+
+def handle_remove_readonly(func: Callable, path: str, exc: ExcInfo) -> None:
+    """Handle errors when trying to remove read-only files through `shutil.rmtree`.
+
+    This handler makes sure the given file is writable, then re-execute the given removal function.
+
+    Arguments:
+        func: An OS-dependant function used to remove a file.
+        path: The path to the file to remove.
+        exc: A `sys.exc_info()` object.
+    """
+    excvalue = exc[1]
+    if func in (os.rmdir, os.remove, os.unlink) and excvalue.errno == errno.EACCES:
+        os.chmod(path, stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)  # 0777
+        func(path)
+    else:
+        raise
+
+
+# See https://github.com/copier-org/copier/issues/345
+class TemporaryDirectory(tempfile.TemporaryDirectory):
+    """A custom version of `tempfile.TemporaryDirectory` that handles read-only files better.
+
+    On Windows, before Python 3.8, `shutil.rmtree` does not handle read-only files very well.
+    This custom class makes use of a [special error handler][copier.tools.handle_remove_readonly]
+    to make sure that a temporary directory containing read-only files (typically created
+    when git-cloning a repository) is properly cleaned-up (i.e. removed) after using it
+    in a context manager.
+    """
+
+    @classmethod
+    def _cleanup(cls, name, warn_message):
+        cls._robust_cleanup(name)
+        warnings.warn(warn_message, ResourceWarning)
+
+    def cleanup(self):
+        if self._finalizer.detach():
+            self._robust_cleanup(self.name)
+
+    @staticmethod
+    def _robust_cleanup(name):
+        shutil.rmtree(name, ignore_errors=False, onerror=handle_remove_readonly)

copier/types.py

@@ -1,6 +1,7 @@
 """Complex types, annotations, validators."""
 
 from pathlib import Path
+from types import TracebackType
 from typing import (
     TYPE_CHECKING,
     Any,
@@ -53,6 +54,7 @@
 Filters = Dict[str, Callable]
 LoaderPaths = Union[str, Iterable[str]]
 VCSTypes = Literal["git"]
+ExcInfo = Tuple[BaseException, Exception, TracebackType]
 
 
 class AllowArbitraryTypes:

copier/vcs.py

@@ -8,6 +8,7 @@
 from plumbum import TF, colors, local
 from plumbum.cmd import git
 
+from .tools import TemporaryDirectory
 from .types import OptBool, OptStr, StrOrPath
 
 GIT_PREFIX = ("git@", "git://", "git+")
@@ -31,7 +32,7 @@ def is_git_repo_root(path: StrOrPath) -> bool:
 
 def is_git_bundle(path: Path) -> bool:
     """Indicate if a path is a valid git bundle."""
-    with tempfile.TemporaryDirectory(prefix=f"{__name__}.is_git_bundle.") as dirname:
+    with TemporaryDirectory(prefix=f"{__name__}.is_git_bundle.") as dirname:
         with local.cwd(dirname):
             git("init")
             return bool(git["bundle", "verify", path] & TF)

tests/test_tools.py

@@ -1,7 +1,11 @@
 from pathlib import Path
+from stat import S_IREAD
 
+from plumbum.cmd import git
 from poethepoet.app import PoeThePoet
 
+from copier.tools import TemporaryDirectory
+
 
 def test_lint():
     """Ensure source code formatting"""
@@ -11,3 +15,20 @@ def test_lint():
 def test_types():
     """Ensure source code static typing."""
     PoeThePoet(Path("."))(["types"])
+
+
+def test_temporary_directory_with_readonly_files_deletion():
+    """Ensure temporary directories containing read-only files are properly deleted, whatever the OS."""
+    with TemporaryDirectory() as tmp_dir:
+        ro_file = Path(tmp_dir) / "readonly.txt"
+        with ro_file.open("w") as fp:
+            fp.write("don't touch me!")
+        ro_file.chmod(S_IREAD)
+    assert not Path(tmp_dir).exists()
+
+
+def test_temporary_directory_with_git_repo_deletion():
+    """Ensure temporary directories containing git repositories are properly deleted, whatever the OS."""
+    with TemporaryDirectory() as tmp_dir:
+        git("clone", "--depth=1", ".", Path(tmp_dir) / "repo")
+    assert not Path(tmp_dir).exists()