From 02c737fe4e823e89af4fc39a88aabda2cceb1e5b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 11 Jul 2019 18:57:03 -0300 Subject: [PATCH] Fix rmtree to remove directories with read-only files (#5588) Fix rmtree to remove directories with read-only files --- changelog/5524.bugfix.rst | 2 ++ src/_pytest/cacheprovider.py | 4 +-- src/_pytest/pathlib.py | 47 ++++++++++++++++++++++------ testing/test_tmpdir.py | 60 ++++++++++++++++++++++++++++++++++-- 4 files changed, 99 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 00000000000..96ebbd43e09 --- /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 045248cb7c2..f5c55454849 100755 --- a/src/_pytest/cacheprovider.py +++ b/src/_pytest/cacheprovider.py @@ -21,7 +21,7 @@ from .compat import _PY2 as PY2 from .pathlib import Path from .pathlib import resolve_from_str -from .pathlib import rmtree +from .pathlib import rm_rf README_CONTENT = u"""\ # pytest cache directory # @@ -51,7 +51,7 @@ class Cache(object): 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 729c41797d9..b502a5200f0 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -1,4 +1,6 @@ # -*- coding: utf-8 -*- +from __future__ import absolute_import + import atexit import errno import fnmatch @@ -8,6 +10,7 @@ import shutil import sys import uuid +import warnings from functools import reduce from os.path import expanduser from os.path import expandvars @@ -19,6 +22,7 @@ from six.moves import map from .compat import PY36 +from _pytest.warning_types import PytestWarning if PY36: from pathlib import Path, PurePath @@ -38,17 +42,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): @@ -186,7 +215,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 40ffe98af98..0c75f74797c 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -3,6 +3,8 @@ from __future__ import division from __future__ import print_function +import os +import stat import sys import attr @@ -318,11 +320,11 @@ def test_cleanup_locked(self, tmp_path): ) 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() @@ -330,9 +332,40 @@ def test_rmtree(self, tmp_path): 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")) @@ -358,3 +391,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(u'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