classification
Title: shutil.rmtree(..., ignore_errors=True) doesn't ignore all errors
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, chris.jerdonek, eng793, eric.araujo, giampaolo.rodola, hynek, jwilk, larry, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: 3.3regression, patch

Created on 2012-09-06 20:07 by jwilk, last changed 2012-12-10 15:37 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
shutil.patch eng793, 2012-09-06 22:35 patch review
shutil.patch eng793, 2012-09-07 00:04 patch review
shutil_rmtree_2.patch serhiy.storchaka, 2012-11-01 18:36 review
shutil_rmtree_4.patch serhiy.storchaka, 2012-11-02 14:43 review
shutil_rmtree_tests-3.2_2.patch serhiy.storchaka, 2012-11-02 14:45 Backported tests for 3.2 review
Messages (21)
msg169936 - (view) Author: Jakub Wilk (jwilk) Date: 2012-09-06 20:07
This used to work correctly in Python 3.2:

Python 3.3.0rc1 (default, Aug 29 2012, 00:39:20) 
[GCC 4.7.1] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import shutil
>>> shutil.rmtree('/etc/fstab', ignore_errors=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.3/shutil.py", line 456, in rmtree
    "Not a directory: '{}'".format(path))
NotADirectoryError: [Errno 20] Not a directory: '/etc/fstab'
msg169948 - (view) Author: Alessandro Moura (eng793) * Date: 2012-09-06 22:35
Yes, confirmed. When checking whether the provided path is a directory, rmtree does not check whether ignore_errors is set. According to the docstring, "If ignore_errors is set, errors are ignored". Presumably this means any error, in which case this is not the desired behaviour.

The attached patch fixes this.
msg169956 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-06 23:27
Can you also provide a test?
msg169957 - (view) Author: Alessandro Moura (eng793) * Date: 2012-09-07 00:04
Added test to patch.
msg173385 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2012-10-20 11:41
+        self.assertEqual(shutil.rmtree(os.path.join(tmpdir, "tstfile"), 
+                                       ignore_errors=True), None)

I wouldn't use assertEqual as the return value is not meaningful.
msg173396 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-10-20 15:12
NotADirectoryError not being caught makes sense to me: not passing a directory as argument to rmtree is a programmer error, not something coming from the OS or filesystem.
msg173397 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-20 15:59
To be honest I don't really understand the point of the ignore_errors flag on rmtree. If rmtree fails to delete the directory tree (which will happen if one of the files can't be deleted), why would you want it to return succesfully?
msg173399 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-20 16:38
Now shutil.rmtree has different behavior when called for non-directory on systems with and without at-functions.
msg173400 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-20 16:42
> If rmtree fails to delete the directory tree (which will happen if one of the files can't be deleted), why would you want it to return succesfully?

At least it free some disk space. ;-)
msg173401 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-10-20 17:48
> To be honest I don't really understand the point of the ignore_errors flag on rmtree. If rmtree fails to delete the directory tree (which will happen if one of the files can't be deleted), why would you want it to return succesfully?

I presume it’s meant as a best-effort cleanup.

Regardless both Eric & Serhiy are right: it’s a programmer error to call it on files and it may shadow bugs catching it. OTOH the implementation is inconsistent and not backward compatible now, so we have to fix it unfortunately.

The patch needs to address Giampaolo’s (bug tracker) & Serhiy’s (Rietveld) comments before it can be merged though – thanks. :)
msg174437 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-01 18:36
Here is a new patch. It contains some other minor changes. rmtree behavior unified for system with and without at-functions.
msg174500 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-02 10:34
Patch updated.  Added tests for onerror.
msg174501 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-02 10:35
Here are backported tests for 3.2 (they are passed).
msg174523 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-02 14:43
Tests simplified.  Thanks Hynek for review and advices.
msg177266 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-10 08:18
New changeset c9b9f786ec25 by Hynek Schlawack in branch '3.2':
#15872: Add tests for a 3.3 regression in the new fd-based shutil.rmtree
http://hg.python.org/cpython/rev/c9b9f786ec25

New changeset fc394216c724 by Hynek Schlawack in branch '3.3':
#15872: Fix 3.3 regression introduced by the new fd-based shutil.rmtree
http://hg.python.org/cpython/rev/fc394216c724

New changeset c70d964b26fe by Hynek Schlawack in branch 'default':
#15872: Fix 3.3 regression introduced by the new fd-based shutil.rmtree
http://hg.python.org/cpython/rev/c70d964b26fe
msg177267 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-10 09:11
New changeset 5211391928bc by Hynek Schlawack in branch '3.2':
#15872: Fix shutil.rmtree error tests for Windows
http://hg.python.org/cpython/rev/5211391928bc

New changeset 4b2fca8ad07b by Hynek Schlawack in branch '3.3':
#15872: Fix shutil.rmtree error tests for Windows
http://hg.python.org/cpython/rev/4b2fca8ad07b

New changeset ae1ef62954f7 by Hynek Schlawack in branch 'default':
#15872: Fix shutil.rmtree error tests for Windows
http://hg.python.org/cpython/rev/ae1ef62954f7
msg177268 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-10 09:29
Thank you, Hynek, for review and committing.
msg177273 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-10 10:27
New changeset 7ce8f4a70ccd by Hynek Schlawack in branch '3.2':
#15872: More shutil test fixes for Windows
http://hg.python.org/cpython/rev/7ce8f4a70ccd

New changeset a05e2d4094ea by Hynek Schlawack in branch '3.3':
#15872: More shutil test fixes for Windows
http://hg.python.org/cpython/rev/a05e2d4094ea

New changeset c23659e2ec1a by Hynek Schlawack in branch 'default':
#15872: More shutil test fixes for Windows
http://hg.python.org/cpython/rev/c23659e2ec1a
msg177275 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-10 11:07
New changeset 2d953d47d634 by Hynek Schlawack in branch '3.2':
#15872: Be flexible with appending *.* in shutil.rmtree test case
http://hg.python.org/cpython/rev/2d953d47d634

New changeset edb747c6c479 by Hynek Schlawack in branch '3.3':
#15872: Be flexible with appending *.* in shutil.rmtree test case
http://hg.python.org/cpython/rev/edb747c6c479

New changeset a0a25ffdec9d by Hynek Schlawack in branch 'default':
#15872: Be flexible with appending *.* in shutil.rmtree test case
http://hg.python.org/cpython/rev/a0a25ffdec9d
msg177287 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-12-10 13:33
“I wish I were wrangling inconsistent Windows buildbots.”

Nobody. Ever. *sigh*

It appears they are appeased now, so finally closing. Thanks for the patches everyone!
msg177296 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-10 15:37
New changeset cb8274e1ebfa by Hynek Schlawack in branch '3.2':
#15872: Some more Windows related tuning to shutil.rmtree tests
http://hg.python.org/cpython/rev/cb8274e1ebfa

New changeset 561c4012929a by Hynek Schlawack in branch '3.3':
#15872: Some more Windows related tuning to shutil.rmtree tests
http://hg.python.org/cpython/rev/561c4012929a

New changeset 451559508c54 by Hynek Schlawack in branch 'default':
#15872: Some more Windows related tuning to shutil.rmtree tests
http://hg.python.org/cpython/rev/451559508c54
History
Date User Action Args
2012-12-10 15:37:11python-devsetmessages: + msg177296
2012-12-10 13:33:11hyneksetstatus: open -> closed
resolution: fixed
messages: + msg177287

stage: patch review -> resolved
2012-12-10 11:07:01python-devsetmessages: + msg177275
2012-12-10 10:27:15python-devsetmessages: + msg177273
2012-12-10 09:31:49serhiy.storchakasetmessages: - msg177269
2012-12-10 09:29:14serhiy.storchakasetmessages: + msg177269
2012-12-10 09:29:07serhiy.storchakasetmessages: + msg177268
2012-12-10 09:11:29python-devsetmessages: + msg177267
2012-12-10 08:18:32python-devsetnosy: + python-dev
messages: + msg177266
2012-11-02 14:45:45serhiy.storchakasetfiles: - shutil_rmtree_tests-3.2.patch
2012-11-02 14:45:22serhiy.storchakasetfiles: - shutil_rmtree_3.patch
2012-11-02 14:45:03serhiy.storchakasetfiles: + shutil_rmtree_tests-3.2_2.patch
2012-11-02 14:43:42serhiy.storchakasetfiles: + shutil_rmtree_4.patch

messages: + msg174523
2012-11-02 10:35:53serhiy.storchakasetfiles: + shutil_rmtree_tests-3.2.patch

messages: + msg174501
2012-11-02 10:34:04serhiy.storchakasetfiles: + shutil_rmtree_3.patch

messages: + msg174500
2012-11-01 18:36:41serhiy.storchakasetfiles: + shutil_rmtree_2.patch

messages: + msg174437
stage: needs patch -> patch review
2012-10-24 09:04:16serhiy.storchakasetstage: needs patch
2012-10-20 17:48:36hyneksetmessages: + msg173401
2012-10-20 16:42:53serhiy.storchakasetmessages: + msg173400
2012-10-20 16:38:09serhiy.storchakasetmessages: + msg173399
2012-10-20 15:59:49pitrousetnosy: + hynek
messages: + msg173397
2012-10-20 15:12:46eric.araujosetnosy: + pitrou, eric.araujo
messages: + msg173396
2012-10-20 11:41:14giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg173385
2012-10-20 08:29:53serhiy.storchakasetkeywords: + 3.3regression
2012-10-20 08:29:23serhiy.storchakasetnosy: + larry, serhiy.storchaka

components: + Library (Lib)
versions: + Python 3.4
2012-10-19 20:13:51barrysetnosy: + barry
2012-09-07 00:04:47eng793setfiles: + shutil.patch

messages: + msg169957
2012-09-06 23:27:09chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg169956
2012-09-06 22:35:52eng793setfiles: + shutil.patch

nosy: + eng793
messages: + msg169948

keywords: + patch
2012-09-06 20:07:15jwilkcreate