Fix rmtree to remove directories with read-only files (pytest-dev#5588)

Fix rmtree to remove directories with read-only files

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.

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)
 

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

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,21 +320,52 @@ 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()
 
  adir.mkdir()
  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