From 37c37963c45b40cb7ef00e4b3ae50b1701604d81 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 10 Jul 2019 09:52:23 -0300 Subject: [PATCH] Fix rmtree to remove directories with read-only files Fix #5524 --- changelog/5524.bugfix.rst | 2 ++ src/_pytest/cacheprovider.py | 4 +-- src/_pytest/pathlib.py | 45 +++++++++++++++++++++------ testing/test_tmpdir.py | 60 ++++++++++++++++++++++++++++++++++-- 4 files changed, 97 insertions(+), 14 deletions(-) create mode 100644 changelog/5524.bugfix.rst diff --git a/changelog/5524.bugfix.rst b/changelog/5524.bugfix.rst new file mode 100644 index 000000000..96ebbd43e --- /dev/null +++ b/changelog/5524.bugfix.rst @@ -0,0 +1,2 @@ +Fix issue where ``tmp_path`` and ``tmpdir`` would not remove directories containing files marked as read-only, +which could lead to pytest crashing when executed a second time with the ``--basetemp`` option. diff --git a/src/_pytest/cacheprovider.py b/src/_pytest/cacheprovider.py index 17463959f..496931e0f 100755 --- a/src/_pytest/cacheprovider.py +++ b/src/_pytest/cacheprovider.py @@ -14,7 +14,7 @@ import py import pytest from .pathlib import Path from .pathlib import resolve_from_str -from .pathlib import rmtree +from .pathlib import rm_rf README_CONTENT = """\ # pytest cache directory # @@ -44,7 +44,7 @@ class Cache: def for_config(cls, config): cachedir = cls.cache_dir_from_config(config) if config.getoption("cacheclear") and cachedir.exists(): - rmtree(cachedir, force=True) + rm_rf(cachedir) cachedir.mkdir() return cls(cachedir, config) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index ecc38eb0f..7094d8c69 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -7,12 +7,14 @@ import os import shutil import sys import uuid +import warnings from os.path import expanduser from os.path import expandvars from os.path import isabs from os.path import sep from posixpath import sep as posix_sep +from _pytest.warning_types import PytestWarning if sys.version_info[:2] >= (3, 6): from pathlib import Path, PurePath @@ -32,17 +34,42 @@ def ensure_reset_dir(path): ensures the given path is an empty directory """ if path.exists(): - rmtree(path, force=True) + rm_rf(path) path.mkdir() -def rmtree(path, force=False): - if force: - # NOTE: ignore_errors might leave dead folders around. - # Python needs a rm -rf as a followup. - shutil.rmtree(str(path), ignore_errors=True) - else: - shutil.rmtree(str(path)) +def rm_rf(path): + """Remove the path contents recursively, even if some elements + are read-only. + """ + + def chmod_w(p): + import stat + + mode = os.stat(str(p)).st_mode + os.chmod(str(p), mode | stat.S_IWRITE) + + def force_writable_and_retry(function, p, excinfo): + p = Path(p) + + # for files, we need to recursively go upwards + # in the directories to ensure they all are also + # writable + if p.is_file(): + for parent in p.parents: + chmod_w(parent) + # stop when we reach the original path passed to rm_rf + if parent == path: + break + + chmod_w(p) + try: + # retry the function that failed + function(str(p)) + except Exception as e: + warnings.warn(PytestWarning("(rm_rf) error removing {}: {}".format(p, e))) + + shutil.rmtree(str(path), onerror=force_writable_and_retry) def find_prefixed(root, prefix): @@ -168,7 +195,7 @@ def maybe_delete_a_numbered_dir(path): garbage = parent.joinpath("garbage-{}".format(uuid.uuid4())) path.rename(garbage) - rmtree(garbage, force=True) + rm_rf(garbage) except (OSError, EnvironmentError): # known races: # * other process did a cleanup at the same time diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index c4c7ebe25..01f9d4652 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -1,3 +1,5 @@ +import os +import stat import sys import attr @@ -311,11 +313,11 @@ class TestNumberedDir: ) def test_rmtree(self, tmp_path): - from _pytest.pathlib import rmtree + from _pytest.pathlib import rm_rf adir = tmp_path / "adir" adir.mkdir() - rmtree(adir) + rm_rf(adir) assert not adir.exists() @@ -323,9 +325,40 @@ class TestNumberedDir: afile = adir / "afile" afile.write_bytes(b"aa") - rmtree(adir, force=True) + rm_rf(adir) assert not adir.exists() + def test_rmtree_with_read_only_file(self, tmp_path): + """Ensure rm_rf can remove directories with read-only files in them (#5524)""" + from _pytest.pathlib import rm_rf + + fn = tmp_path / "dir/foo.txt" + fn.parent.mkdir() + + fn.touch() + + mode = os.stat(str(fn)).st_mode + os.chmod(str(fn), mode & ~stat.S_IWRITE) + + rm_rf(fn.parent) + + assert not fn.parent.is_dir() + + def test_rmtree_with_read_only_directory(self, tmp_path): + """Ensure rm_rf can remove read-only directories (#5524)""" + from _pytest.pathlib import rm_rf + + adir = tmp_path / "dir" + adir.mkdir() + + (adir / "foo.txt").touch() + mode = os.stat(str(adir)).st_mode + os.chmod(str(adir), mode & ~stat.S_IWRITE) + + rm_rf(adir) + + assert not adir.is_dir() + def test_cleanup_ignores_symlink(self, tmp_path): the_symlink = tmp_path / (self.PREFIX + "current") attempt_symlink_to(the_symlink, tmp_path / (self.PREFIX + "5")) @@ -349,3 +382,24 @@ def attempt_symlink_to(path, to_path): def test_tmpdir_equals_tmp_path(tmpdir, tmp_path): assert Path(tmpdir) == tmp_path + + +def test_basetemp_with_read_only_files(testdir): + """Integration test for #5524""" + testdir.makepyfile( + """ + import os + import stat + + def test(tmp_path): + fn = tmp_path / 'foo.txt' + fn.write_text('hello') + mode = os.stat(str(fn)).st_mode + os.chmod(str(fn), mode & ~stat.S_IREAD) + """ + ) + result = testdir.runpytest("--basetemp=tmp") + assert result.ret == 0 + # running a second time and ensure we don't crash + result = testdir.runpytest("--basetemp=tmp") + assert result.ret == 0