Repository: pypa/setuptools
Pull request #4682 (2024-10-15 16:34:03)

Refactor/unify/extract `shutil.rmtree` callbacks (and avoid repetition)
<!-- First time contributors: Take a moment to review https://setuptools.pypa.io/en/latest/development/developer-guide.html! -->
<!-- Remove sections if not applicable -->

## Summary of changes

I noticed that there are at least 2 implementations of `shutil.rmtree` callbacks in setuptools: one for `easy_install` and one for `bdist_wheel`. There is also other places in setuptools that we don't have those callbacks (so potentially still subject to errors on Windows?).

So this PR tries to unify these 2 separated implementations.
I have chosen to extract the implementation from `easy_install`.
One notable difference though is that `bdist_wheel`'s implementation always try to apply the `chmod` callback when it fails, while the `easy_install` implementation is more conservative and only applies the callback on Windows (similar to the suggestion in `https://github.com/python/cpython/issues/87823#issuecomment-1093908280`)... So that might generate a bit of change in behaviour, but with a bit of luck should be compatible.

The name `_shutil` was used for the lack of a better name/imagination, and can be changed.


<!-- Summary goes here -->

Closes <!-- issue number here -->

### Pull Request Checklist
- [ ] Changes have tests
- [ ] News fragment added in [`newsfragments/`].
  _(See [documentation][PR docs] for details)_


[`newsfragments/`]: https://github.com/pypa/setuptools/tree/master/newsfragments
[PR docs]:
https://setuptools.pypa.io/en/latest/development/developer-guide.html#making-a-pull-request



Patch:
diff --git a/setuptools/_shutil.py b/setuptools/_shutil.py
new file mode 100644
index 000000000..6acbb4281
--- /dev/null
+++ b/setuptools/_shutil.py
@@ -0,0 +1,53 @@
+"""Convenience layer on top of stdlib's shutil and os"""
+
+import os
+import stat
+from typing import Callable, TypeVar
+
+from .compat import py311
+
+from distutils import log
+
+try:
+    from os import chmod  # pyright: ignore[reportAssignmentType]
+    # Losing type-safety w/ pyright, but that's ok
+except ImportError:  # pragma: no cover
+    # Jython compatibility
+    def chmod(*args: object, **kwargs: object) -> None:  # type: ignore[misc] # Mypy reuses the imported definition anyway
+        pass
+
+
+_T = TypeVar("_T")
+
+
+def attempt_chmod_verbose(path, mode):
+    log.debug("changing mode of %s to %o", path, mode)
+    try:
+        chmod(path, mode)
+    except OSError as e:  # pragma: no cover
+        log.debug("chmod failed: %s", e)
+
+
+# Must match shutil._OnExcCallback
+def _auto_chmod(
+    func: Callable[..., _T], arg: str, exc: BaseException
+) -> _T:  # pragma: no cover
+    """shutils onexc callback to automatically call chmod for certain functions."""
+    # Only retry for scenarios known to have an issue
+    if func in [os.unlink, os.remove] and os.name == 'nt':
+        attempt_chmod_verbose(arg, stat.S_IWRITE)
+        return func(arg)
+    raise exc
+
+
+def rmtree(path, ignore_errors=False, onexc=_auto_chmod):
+    """
+    Similar to ``shutil.rmtree`` but automatically executes ``chmod``
+    for well know Windows failure scenarios.
+    """
+    return py311.shutil_rmtree(path, ignore_errors, onexc)
+
+
+def rmdir(path, **opts):
+    if os.path.isdir(path):
+        rmtree(path, **opts)
diff --git a/setuptools/command/bdist_wheel.py b/setuptools/command/bdist_wheel.py
index 8cf91538f..5855a8a83 100644
--- a/setuptools/command/bdist_wheel.py
+++ b/setuptools/command/bdist_wheel.py
@@ -9,7 +9,6 @@ from __future__ import annotations
 import os
 import re
 import shutil
-import stat
 import struct
 import sys
 import sysconfig
@@ -18,23 +17,19 @@ from collections.abc import Iterable, Sequence
 from email.generator import BytesGenerator, Generator
 from email.policy import EmailPolicy
 from glob import iglob
-from shutil import rmtree
-from typing import TYPE_CHECKING, Callable, Literal, cast
+from typing import Literal, cast
 from zipfile import ZIP_DEFLATED, ZIP_STORED
 
 from packaging import tags, version as _packaging_version
 from wheel.metadata import pkginfo_to_metadata
 from wheel.wheelfile import WheelFile
 
-from .. import Command, __version__
+from .. import Command, __version__, _shutil
 from ..warnings import SetuptoolsDeprecationWarning
 from .egg_info import egg_info as egg_info_cls
 
 from distutils import log
 
-if TYPE_CHECKING:
-    from _typeshed import ExcInfo
-
 
 def safe_name(name: str) -> str:
     """Convert an arbitrary string to a standard distribution name
@@ -148,21 +143,6 @@ def safer_version(version: str) -> str:
     return safe_version(version).replace("-", "_")
 
 
-def remove_readonly(
-    func: Callable[..., object],
-    path: str,
-    excinfo: ExcInfo,
-) -> None:
-    remove_readonly_exc(func, path, excinfo[1])
-
-
-def remove_readonly_exc(
-    func: Callable[..., object], path: str, exc: BaseException
-) -> None:
-    os.chmod(path, stat.S_IWRITE)
-    func(path)
-
-
 class bdist_wheel(Command):
     description = "create a wheel distribution"
 
@@ -458,7 +438,7 @@ class bdist_wheel(Command):
             shutil.copytree(self.dist_info_dir, distinfo_dir)
             # Egg info is still generated, so remove it now to avoid it getting
             # copied into the wheel.
-            shutil.rmtree(self.egginfo_dir)
+            _shutil.rmtree(self.egginfo_dir)
         else:
             # Convert the generated egg-info into dist-info.
             self.egg2dist(self.egginfo_dir, distinfo_dir)
@@ -483,10 +463,7 @@ class bdist_wheel(Command):
         if not self.keep_temp:
             log.info(f"removing {self.bdist_dir}")
             if not self.dry_run:
-                if sys.version_info < (3, 12):
-                    rmtree(self.bdist_dir, onerror=remove_readonly)
-                else:
-                    rmtree(self.bdist_dir, onexc=remove_readonly_exc)
+                _shutil.rmtree(self.bdist_dir)
 
     def write_wheelfile(
         self, wheelfile_base: str, generator: str = f"setuptools ({__version__})"
@@ -570,7 +547,7 @@ class bdist_wheel(Command):
         def adios(p: str) -> None:
             """Appropriately delete directory, file or link."""
             if os.path.exists(p) and not os.path.islink(p) and os.path.isdir(p):
-                shutil.rmtree(p)
+                _shutil.rmtree(p)
             elif os.path.exists(p):
                 os.unlink(p)
 
diff --git a/setuptools/command/dist_info.py b/setuptools/command/dist_info.py
index 3ad27ed70..0192ebb26 100644
--- a/setuptools/command/dist_info.py
+++ b/setuptools/command/dist_info.py
@@ -10,6 +10,7 @@ from pathlib import Path
 from typing import cast
 
 from .. import _normalization
+from .._shutil import rmdir as _rm
 from .egg_info import egg_info as egg_info_cls
 
 from distutils import log
@@ -100,8 +101,3 @@ class dist_info(Command):
         # TODO: if bdist_wheel if merged into setuptools, just add "keep_egg_info" there
         with self._maybe_bkp_dir(egg_info_dir, self.keep_egg_info):
             bdist_wheel.egg2dist(egg_info_dir, self.dist_info_dir)
-
-
-def _rm(dir_name, **opts):
-    if os.path.isdir(dir_name):
-        shutil.rmtree(dir_name, **opts)
diff --git a/setuptools/command/easy_install.py b/setuptools/command/easy_install.py
index 21e6f008d..b40610f8b 100644
--- a/setuptools/command/easy_install.py
+++ b/setuptools/command/easy_install.py
@@ -34,7 +34,7 @@ import zipimport
 from collections.abc import Iterable
 from glob import glob
 from sysconfig import get_path
-from typing import TYPE_CHECKING, Callable, NoReturn, TypedDict, TypeVar
+from typing import TYPE_CHECKING, NoReturn, TypedDict
 
 from jaraco.text import yield_lines
 
@@ -63,7 +63,8 @@ from setuptools.warnings import SetuptoolsDeprecationWarning, SetuptoolsWarning
 from setuptools.wheel import Wheel
 
 from .._path import ensure_directory
-from ..compat import py39, py311, py312
+from .._shutil import attempt_chmod_verbose as chmod, rmtree as _rmtree
+from ..compat import py39, py312
 
 from distutils import dir_util, log
 from distutils.command import install
@@ -89,8 +90,6 @@ __all__ = [
     'get_exe_prefixes',
 ]
 
-_T = TypeVar("_T")
-
 
 def is_64bit():
     return struct.calcsize("P") == 8
@@ -1789,16 +1788,6 @@ def _first_line_re():
     return re.compile(first_line_re.pattern.decode())
 
 
-# Must match shutil._OnExcCallback
-def auto_chmod(func: Callable[..., _T], arg: str, exc: BaseException) -> _T:
-    """shutils onexc callback to automatically call chmod for certain functions."""
-    # Only retry for scenarios known to have an issue
-    if func in [os.unlink, os.remove] and os.name == 'nt':
-        chmod(arg, stat.S_IWRITE)
-        return func(arg)
-    raise exc
-
-
 def update_dist_caches(dist_path, fix_zipimporter_caches):
     """
     Fix any globally cached `dist_path` related data
@@ -2021,24 +2010,6 @@ def is_python_script(script_text, filename):
     return False  # Not any Python I can recognize
 
 
-try:
-    from os import (
-        chmod as _chmod,  # pyright: ignore[reportAssignmentType] # Losing type-safety w/ pyright, but that's ok
-    )
-except ImportError:
-    # Jython compatibility
-    def _chmod(*args: object, **kwargs: object) -> None:  # type: ignore[misc] # Mypy reuses the imported definition anyway
-        pass
-
-
-def chmod(path, mode):
-    log.debug("changing mode of %s to %o", path, mode)
-    try:
-        _chmod(path, mode)
-    except OSError as e:
-        log.debug("chmod failed: %s", e)
-
-
 class _SplitArgs(TypedDict, total=False):
     comments: bool
     posix: bool
@@ -2350,10 +2321,6 @@ def load_launcher_manifest(name):
     return manifest.decode('utf-8') % vars()
 
 
-def _rmtree(path, ignore_errors: bool = False, onexc=auto_chmod):
-    return py311.shutil_rmtree(path, ignore_errors, onexc)
-
-
 def current_umask():
     tmp = os.umask(0o022)
     os.umask(tmp)
diff --git a/setuptools/command/editable_wheel.py b/setuptools/command/editable_wheel.py
index 30570e092..db9a50c3a 100644
--- a/setuptools/command/editable_wheel.py
+++ b/setuptools/command/editable_wheel.py
@@ -27,7 +27,7 @@ from tempfile import TemporaryDirectory
 from types import TracebackType
 from typing import TYPE_CHECKING, Protocol, TypeVar, cast
 
-from .. import Command, _normalization, _path, errors, namespaces
+from .. import Command, _normalization, _path, _shutil, errors, namespaces
 from .._path import StrPath
 from ..compat import py312
 from ..discovery import find_package_path
@@ -773,7 +773,7 @@ def _is_nested(pkg: str, pkg_path: str, parent: str, parent_path: str) -> bool:
 
 def _empty_dir(dir_: _P) -> _P:
     """Create a directory ensured to be empty. Existing files may be removed."""
-    shutil.rmtree(dir_, ignore_errors=True)
+    _shutil.rmtree(dir_, ignore_errors=True)
     os.makedirs(dir_)
     return dir_
 
diff --git a/setuptools/command/rotate.py b/setuptools/command/rotate.py
index c10e8d502..acdce07ba 100644
--- a/setuptools/command/rotate.py
+++ b/setuptools/command/rotate.py
@@ -1,10 +1,9 @@
 from __future__ import annotations
 
 import os
-import shutil
 from typing import ClassVar
 
-from setuptools import Command
+from .. import Command, _shutil
 
 from distutils import log
 from distutils.errors import DistutilsOptionError
@@ -61,6 +60,6 @@ class rotate(Command):
                 log.info("Deleting %s", f)
                 if not self.dry_run:
                     if os.path.isdir(f):
-                        shutil.rmtree(f)
+                        _shutil.rmtree(f)
                     else:
                         os.unlink(f)


Test patch:
diff --git a/setuptools/tests/test_bdist_wheel.py b/setuptools/tests/test_bdist_wheel.py
index 3dfa9c850..d51dfbeb6 100644
--- a/setuptools/tests/test_bdist_wheel.py
+++ b/setuptools/tests/test_bdist_wheel.py
@@ -11,7 +11,6 @@ import sys
 import sysconfig
 from contextlib import suppress
 from inspect import cleandoc
-from unittest.mock import Mock
 from zipfile import ZipFile
 
 import jaraco.path
@@ -19,12 +18,7 @@ import pytest
 from packaging import tags
 
 import setuptools
-from setuptools.command.bdist_wheel import (
-    bdist_wheel,
-    get_abi_tag,
-    remove_readonly,
-    remove_readonly_exc,
-)
+from setuptools.command.bdist_wheel import bdist_wheel, get_abi_tag
 from setuptools.dist import Distribution
 from setuptools.warnings import SetuptoolsDeprecationWarning
 
@@ -510,29 +504,6 @@ def test_platform_with_space(dummy_dist, monkeypatch):
     bdist_wheel_cmd(plat_name="isilon onefs").run()
 
 
-def test_rmtree_readonly(monkeypatch, tmp_path):
-    """Verify onerr works as expected"""
-
-    bdist_dir = tmp_path / "with_readonly"
-    bdist_dir.mkdir()
-    some_file = bdist_dir.joinpath("file.txt")
-    some_file.touch()
-    some_file.chmod(stat.S_IREAD)
-
-    expected_count = 1 if sys.platform.startswith("win") else 0
-
-    if sys.version_info < (3, 12):
-        count_remove_readonly = Mock(side_effect=remove_readonly)
-        shutil.rmtree(bdist_dir, onerror=count_remove_readonly)
-        assert count_remove_readonly.call_count == expected_count
-    else:
-        count_remove_readonly_exc = Mock(side_effect=remove_readonly_exc)
-        shutil.rmtree(bdist_dir, onexc=count_remove_readonly_exc)
-        assert count_remove_readonly_exc.call_count == expected_count
-
-    assert not bdist_dir.is_dir()
-
-
 def test_data_dir_with_tag_build(monkeypatch, tmp_path):
     """
     Setuptools allow authors to set PEP 440's local version segments
diff --git a/setuptools/tests/test_shutil_wrapper.py b/setuptools/tests/test_shutil_wrapper.py
new file mode 100644
index 000000000..74ff7e9a8
--- /dev/null
+++ b/setuptools/tests/test_shutil_wrapper.py
@@ -0,0 +1,23 @@
+import stat
+import sys
+from unittest.mock import Mock
+
+from setuptools import _shutil
+
+
+def test_rmtree_readonly(monkeypatch, tmp_path):
+    """Verify onerr works as expected"""
+
+    tmp_dir = tmp_path / "with_readonly"
+    tmp_dir.mkdir()
+    some_file = tmp_dir.joinpath("file.txt")
+    some_file.touch()
+    some_file.chmod(stat.S_IREAD)
+
+    expected_count = 1 if sys.platform.startswith("win") else 0
+    chmod_fn = Mock(wraps=_shutil.attempt_chmod_verbose)
+    monkeypatch.setattr(_shutil, "attempt_chmod_verbose", chmod_fn)
+
+    _shutil.rmtree(tmp_dir)
+    assert chmod_fn.call_count == expected_count
+    assert not tmp_dir.is_dir()