From 3a03d81669fd5f173983298183d4e8ff48e37f2d Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 19 Aug 2024 21:17:10 +0100 Subject: [PATCH 1/5] GH-73991: Prune `pathlib.Path.delete()` Remove the *ignore_errors* and *on_error* arguments from `Path.delete()`. This functionality was carried over from `shutil`, but its design needs to be re-considered in its new context. For example, we may wish to support a *missing_ok* argument (like `Path.unlink()`), or automatically `chmod()` and retry operations when we hit a permission error (like `tempfile.TemporaryDirectory`), or retry operations with a backoff (like `test.support.os_helper.rmtree()`), or utilise exception groups, etc. It's best to leave our options open for now. --- Doc/library/pathlib.rst | 11 +--- Lib/pathlib/_abc.py | 23 ++------ Lib/pathlib/_local.py | 24 +------- Lib/test/test_pathlib/test_pathlib.py | 72 +---------------------- Lib/test/test_pathlib/test_pathlib_abc.py | 8 --- 5 files changed, 9 insertions(+), 129 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index f66d36a32cbd04..6df481e3005db9 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1637,20 +1637,11 @@ Copying, renaming and deleting :meth:`Path.delete` to remove a non-empty directory. -.. method:: Path.delete(ignore_errors=False, on_error=None) +.. method:: Path.delete() Delete this file or directory. If this path refers to a non-empty directory, its files and sub-directories are deleted recursively. - If *ignore_errors* is true, errors resulting from failed deletions will be - ignored. If *ignore_errors* is false or omitted, and a callable is given as - the optional *on_error* argument, it will be called with one argument of - type :exc:`OSError` each time an exception is raised. The callable can - handle the error to continue the deletion process or re-raise it to stop. - Note that the filename is available as the :attr:`~OSError.filename` - attribute of the exception object. If neither *ignore_errors* nor - *on_error* are supplied, exceptions are propagated to the caller. - .. note:: When deleting non-empty directories on platforms that lack the necessary diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 720756cac66f68..c7d2b27d01adfc 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -923,23 +923,13 @@ def rmdir(self): """ raise UnsupportedOperation(self._unsupported_msg('rmdir()')) - def delete(self, ignore_errors=False, on_error=None): + def delete(self): """ Delete this file or directory (including all sub-directories). - - If *ignore_errors* is true, exceptions raised from scanning the - filesystem and removing files and directories are ignored. Otherwise, - if *on_error* is set, it will be called to handle the error. If - neither *ignore_errors* nor *on_error* are set, exceptions are - propagated to the caller. """ - if ignore_errors: - def on_error(err): - pass - elif on_error is None: + if self.is_dir(follow_symlinks=False): def on_error(err): raise err - if self.is_dir(follow_symlinks=False): results = self.walk( on_error=on_error, top_down=False, # So we rmdir() empty directories. @@ -955,14 +945,9 @@ def on_error(err): dirpath.joinpath(name).rmdir() except OSError as err: on_error(err) - delete_self = self.rmdir + self.rmdir() else: - delete_self = self.unlink - try: - delete_self() - except OSError as err: - err.filename = str(self) - on_error(err) + self.unlink() delete.avoids_symlink_attacks = False def owner(self, *, follow_symlinks=True): diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 8f5c58c16ef0d0..e5e45fb1cbe2a3 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -824,32 +824,14 @@ def rmdir(self): """ os.rmdir(self) - def delete(self, ignore_errors=False, on_error=None): + def delete(self): """ Delete this file or directory (including all sub-directories). - - If *ignore_errors* is true, exceptions raised from scanning the - filesystem and removing files and directories are ignored. Otherwise, - if *on_error* is set, it will be called to handle the error. If - neither *ignore_errors* nor *on_error* are set, exceptions are - propagated to the caller. """ if self.is_dir(follow_symlinks=False): - onexc = None - if on_error: - def onexc(func, filename, err): - err.filename = filename - on_error(err) - shutil.rmtree(str(self), ignore_errors, onexc=onexc) + shutil.rmtree(str(self)) else: - try: - self.unlink() - except OSError as err: - if not ignore_errors: - if on_error: - on_error(err) - else: - raise + self.unlink() delete.avoids_symlink_attacks = shutil.rmtree.avoids_symlink_attacks diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 4c60f278834c9c..67c161237747a1 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -904,10 +904,7 @@ def test_delete_unwritable(self): child_dir_path.chmod(new_mode) tmp.chmod(new_mode) - errors = [] - tmp.delete(on_error=errors.append) - # Test whether onerror has actually been called. - self.assertEqual(len(errors), 3) + self.assertRaises(PermissionError, tmp.delete) finally: tmp.chmod(old_dir_mode) child_file_path.chmod(old_child_file_mode) @@ -1003,16 +1000,6 @@ def close(fd): self.assertTrue(dir2.is_dir()) self.assertEqual(close_count, 2) - close_count = 0 - errors = [] - - with swap_attr(os, 'close', close) as orig_close: - dir1.delete(on_error=errors.append) - self.assertEqual(len(errors), 2) - self.assertEqual(errors[0].filename, str(dir2)) - self.assertEqual(errors[1].filename, str(dir1)) - self.assertEqual(close_count, 2) - @unittest.skipUnless(hasattr(os, "mkfifo"), 'requires os.mkfifo()') @unittest.skipIf(sys.platform == "vxworks", "fifo requires special path on VxWorks") @@ -1028,63 +1015,6 @@ def test_delete_on_named_pipe(self): p.delete() self.assertFalse(p.exists()) - @unittest.skipIf(sys.platform[:6] == 'cygwin', - "This test can't be run on Cygwin (issue #1071513).") - @os_helper.skip_if_dac_override - @os_helper.skip_unless_working_chmod - def test_delete_deleted_race_condition(self): - # bpo-37260 - # - # Test that a file or a directory deleted after it is enumerated - # by scandir() but before unlink() or rmdr() is called doesn't - # generate any errors. - def on_error(exc): - assert exc.filename - if not isinstance(exc, PermissionError): - raise - # Make the parent and the children writeable. - for p, mode in zip(paths, old_modes): - p.chmod(mode) - # Remove other dirs except one. - keep = next(p for p in dirs if str(p) != exc.filename) - for p in dirs: - if p != keep: - p.rmdir() - # Remove other files except one. - keep = next(p for p in files if str(p) != exc.filename) - for p in files: - if p != keep: - p.unlink() - - tmp = self.cls(self.base, 'delete') - tmp.mkdir() - paths = [tmp] + [tmp / f'child{i}' for i in range(6)] - dirs = paths[1::2] - files = paths[2::2] - for path in dirs: - path.mkdir() - for path in files: - path.write_text('') - - old_modes = [path.stat().st_mode for path in paths] - - # Make the parent and the children non-writeable. - new_mode = stat.S_IREAD | stat.S_IEXEC - for path in reversed(paths): - path.chmod(new_mode) - - try: - tmp.delete(on_error=on_error) - except: - # Test failed, so cleanup artifacts. - for path, mode in zip(paths, old_modes): - try: - path.chmod(mode) - except OSError: - pass - tmp.delete() - raise - def test_delete_does_not_choke_on_failing_lstat(self): try: orig_lstat = os.lstat diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index f222fd5b1ec082..e45fbe330d9da2 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -2705,14 +2705,6 @@ def test_delete_missing(self): # filename is guaranteed not to exist filename = tmp / 'foo' self.assertRaises(FileNotFoundError, filename.delete) - # test that ignore_errors option is honored - filename.delete(ignore_errors=True) - # test on_error - errors = [] - filename.delete(on_error=errors.append) - self.assertEqual(len(errors), 1) - self.assertIsInstance(errors[0], FileNotFoundError) - self.assertEqual(errors[0].filename, str(filename)) def setUpWalk(self): # Build: From 2420b85cb6adcd896e4217ea1bf3f3ae7fe2af5c Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 19 Aug 2024 21:47:43 +0100 Subject: [PATCH 2/5] Windows tests fixes --- Lib/test/test_pathlib/test_pathlib.py | 48 ++++----------------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 67c161237747a1..daadfb522c90b3 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -715,11 +715,6 @@ def test_copy_dir_no_read_permission(self): target = base / 'copyE' self.assertRaises(PermissionError, source.copy, target) self.assertFalse(target.exists()) - errors = [] - source.copy(target, on_error=errors.append) - self.assertEqual(len(errors), 1) - self.assertIsInstance(errors[0], PermissionError) - self.assertFalse(target.exists()) def test_copy_dir_preserve_metadata(self): base = self.cls(self.base) @@ -939,42 +934,13 @@ def test_delete_outer_junction(self): import _winapi tmp = self.cls(self.base, 'delete') tmp.mkdir() - try: - src = tmp / 'cheese' - dst = tmp / 'shop' - src.mkdir() - spam = src / 'spam' - spam.write_text('') - _winapi.CreateJunction(str(src), str(dst)) - self.assertRaises(OSError, dst.delete) - dst.delete(ignore_errors=True) - finally: - tmp.delete(ignore_errors=True) - - @needs_windows - def test_delete_outer_junction_on_error(self): - import _winapi - tmp = self.cls(self.base, 'delete') - tmp.mkdir() - dir_ = tmp / 'dir' - dir_.mkdir() - link = tmp / 'link' - _winapi.CreateJunction(str(dir_), str(link)) - try: - self.assertRaises(OSError, link.delete) - self.assertTrue(dir_.exists()) - self.assertTrue(link.exists(follow_symlinks=False)) - errors = [] - - def on_error(error): - errors.append(error) - - link.delete(on_error=on_error) - self.assertEqual(len(errors), 1) - self.assertIsInstance(errors[0], OSError) - self.assertEqual(errors[0].filename, str(link)) - finally: - os.unlink(str(link)) + src = tmp / 'cheese' + dst = tmp / 'shop' + src.mkdir() + spam = src / 'spam' + spam.write_text('') + _winapi.CreateJunction(str(src), str(dst)) + self.assertRaises(OSError, dst.delete) @unittest.skipUnless(delete_use_fd_functions, "requires safe delete") def test_delete_fails_on_close(self): From b934731438db953a2131113abcaa94bb0311a6be Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 19 Aug 2024 23:02:49 +0100 Subject: [PATCH 3/5] Improve deletion of junctions --- Lib/pathlib/_abc.py | 4 +++- Lib/test/test_pathlib/test_pathlib.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index c7d2b27d01adfc..e925d57ccbf081 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -927,7 +927,9 @@ def delete(self): """ Delete this file or directory (including all sub-directories). """ - if self.is_dir(follow_symlinks=False): + if self.is_symlink() or self.is_junction(): + self.unlink() + elif self.is_dir(): def on_error(err): raise err results = self.walk( diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index daadfb522c90b3..67b0cd125c4d4b 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -940,7 +940,10 @@ def test_delete_outer_junction(self): spam = src / 'spam' spam.write_text('') _winapi.CreateJunction(str(src), str(dst)) - self.assertRaises(OSError, dst.delete) + dst.delete() + self.assertFalse(dst.exists()) + self.assertTrue(spam.exists()) + self.assertTrue(src.exists()) @unittest.skipUnless(delete_use_fd_functions, "requires safe delete") def test_delete_fails_on_close(self): From 8edcabecf4070311069e0bac49094ba9f7573ac2 Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 19 Aug 2024 23:29:36 +0100 Subject: [PATCH 4/5] Junction handling tweaks --- Lib/pathlib/_local.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index e5e45fb1cbe2a3..f7fe25685fd89d 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -828,7 +828,9 @@ def delete(self): """ Delete this file or directory (including all sub-directories). """ - if self.is_dir(follow_symlinks=False): + if self.is_symlink() or self.is_junction(): + self.unlink() + elif self.is_dir(): shutil.rmtree(str(self)) else: self.unlink() From 229b60ae0d753ca4b8424043b5b34151a36ec625 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 20 Aug 2024 23:02:35 +0100 Subject: [PATCH 5/5] Undo unnecessary change. --- Lib/pathlib/_abc.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index e925d57ccbf081..c7d2b27d01adfc 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -927,9 +927,7 @@ def delete(self): """ Delete this file or directory (including all sub-directories). """ - if self.is_symlink() or self.is_junction(): - self.unlink() - elif self.is_dir(): + if self.is_dir(follow_symlinks=False): def on_error(err): raise err results = self.walk(