msg203285 - (view) |
Author: Ivan Radic (ivan.radic) |
Date: 2013-11-18 12:11 |
shutil.rmtree works nice on Windows until it hits file with read only attribute set. Workaround is to provide a onerror parameter as a function that checks and removes file attribute before attempting to delete it. Can option to delete read_only files be integrated in shutil.rmtree?
Example output in In Python 2.7:
shutil.rmtree("C:\\2")
Traceback (most recent call last):
File "<pyshell#60>", line 1, in <module>
shutil.rmtree("C:\\2")
File "C:\Program Files (x86)\Python.2.7.3\lib\shutil.py", line 250, in rmtree
onerror(os.remove, fullname, sys.exc_info())
File "C:\Program Files (x86)\Python.2.7.3\lib\shutil.py", line 248, in rmtree
os.remove(fullname)
WindowsError: [Error 5] Access is denied: 'C:\\2\\read_only_file.txt'
Example output in In Python 3.3:
shutil.rmtree("C:\\2")
Traceback (most recent call last):
File "<pyshell#1>", line 1, in <module>
shutil.rmtree("C:\\2")
File "C:\Program Files (x86)\Python.3.3.0\lib\shutil.py", line 460, in rmtree
return _rmtree_unsafe(path, onerror)
File "C:\Program Files (x86)\Python.3.3.0\lib\shutil.py", line 367, in _rmtree_unsafe
onerror(os.unlink, fullname, sys.exc_info())
File "C:\Program Files (x86)\Python.3.3.0\lib\shutil.py", line 365, in _rmtree_unsafe
os.unlink(fullname)
PermissionError: [WinError 5] Access is denied: 'C:\\2\\read_only_file.txt'
|
msg203297 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2013-11-18 14:53 |
You are essentially asking that we have an option to make the windows behavior mirror the posix behavior? (A read only file in a writable directory can be deleted in posix, since only the directory entry, not the file, is being deleted.)
That makes some sense to me. I wonder what the windows devs think?
|
msg203298 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2013-11-18 15:08 |
This, unfortunately, is the classic edge-case where intra-platform consistency and inter-platform consistency clash.
I (on Windows) would certainly be surprised if a tree delete removed read-only files without my specifying some kind of override. I understand why cross-platform behaviour might be preferred, and I'm open to being convinced, but I start at -0.
|
msg203301 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2013-11-18 15:34 |
Well, it would *definitely* need to be a new explicit option whose default value was the current behavior.
|
msg203308 - (view) |
Author: Ivan Radic (ivan.radic) |
Date: 2013-11-18 16:54 |
>You are essentially asking that we have an option to make the windows behavior mirror the posix behavior? (A read only file in a writable directory can be deleted in posix, since only the directory entry, not the file, is being deleted.)
Actually, your explanation is perfect.
I want to be able to remove some directory after I am done using it. When similar operation is done through file manager, dialog pops up asking for confirmation, I would like to have function parameter equivalent of "yes to all" dialog that file manager gives me.
The thing is, anyone working with files is used to think in "rm -rf" kind of way, and on Windows read_only files break this workflow. I discovered this problem few days ago when I was working on custom backup script that needs to work both on Linux (at home) and Windows (at work). Currently, I need to have some extra *windows only* code just to be able to successfully remove a directory.
Quick Google search discovered the workaround (http://stackoverflow.com/questions/1889597/deleting-directory-in-python), so I am set, but the original
question: "Why oh why is this such a pain?"
and the comment: "Maybe nobody has taken the five minutes to file a bug at bugs.python.org"
resonated in my head long enough to give it a try.
For me it makes sense to have this option configurable. And it make a ton of sense to support one line equivalent of "rm -rf".
|
msg203313 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2013-11-18 19:06 |
I like the idea of a remove_readonly flag. I was going to say that I'm a bit worried about the fact that shutil.rmtree already has a couple of keyword arguments, but it's nowhere near what, say, copytree has. Call me +0.75.
|
msg203314 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2013-11-18 19:24 |
That's not a good name for the flag. The problem is that 'read-only' means different things on Windows than it does on Unix. Which is why I suggested that the flag control whether or not it acts "posix like" on Windows. So perhaps 'posix_compat'? That feels a bit odd, though, since on posix it does nothing...so it's really about behavioral consistency across platforms....
Hmm. It's really hard to think of a name that conveys succinctly what we are talking about here.
A more radical notion would be something like 'delete_control' and have it be tri-valued: 'unixlike', 'windowslike', and 'native', with the default being native. Bad names, most likely, but you get the idea. The disadvantage is that it would be even more code to implement ;)
|
msg203315 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2013-11-18 19:32 |
TBH I'm still fairly -0 and edging towards -0.5. If we didn't already
have two keyword args I might be convinced towards a jfdi=True flag.
But, as the OP described, the current params already allow for a
workaround of sorts and another param of the semantics we're discussing
would surely just be confusing?
(Or you could just "preprocess" on Windows, eg attrib -r /s)
|
msg203316 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2013-11-18 19:33 |
I make no claims of being good at naming things :)
|
msg208647 - (view) |
Author: Paul Moore (paul.moore) * |
Date: 2014-01-21 14:44 |
The most obvious solution would be if the onerror argument allowed for retries. At the moment, all it can do is report issues, not recover. Suppose that returning True from onerror meant "retry the operation". Then you could do
def set_rw(operation, name, exc):
os.chmod(name, stat.S_IWRITE)
return True
shutil.rmtree('path', onerror=set_rw)
|
msg208651 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-01-21 15:17 |
See issue 8523 for a discussion of changing the way onerror behaves. I think it is addressing this same use case, but I didn't reread it in detail.
|
msg208654 - (view) |
Author: Paul Moore (paul.moore) * |
Date: 2014-01-21 15:29 |
It's similar. But the problem is that it only returns a list of errors, it doesn't let you address the error *while the rmtree is in progress*.
The key thing is that if you can fix the problem in onerror, you can avoid needing to restart the whole tree walk, which is the key aspect of rmtree.
As things stand, you can use the set_rw function I showed above, and run the rmtree twice:
shutil.rmtree('path', onerror=set_rw)
shutil.rmtree('path')
The first run fixes the error and then the second one deletes the remaining files. But this is clearly inefficient, and makes the limitations of "report errors to the user who can then address them" fairly obvious.
|
msg208657 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-01-21 15:50 |
OK, rereading that issue, I disagree with Tarek and I think that the patch on that issue is ill-advised as it looks like it changes behavior in a non-backward-compatible way.
If you changed your set_rw onerror handler to a rm_ro_file error handler, would things work? That is, have the handler delete the file?
Adding a 'retry' capability is interesting, but would be a non-trivial change in logic, and should be addressed in a new issue, not this one.
|
msg208662 - (view) |
Author: Paul Moore (paul.moore) * |
Date: 2014-01-21 16:13 |
Looks like that works. At least in my case - I just did
def del_rw(action, name, exc):
os.chmod(name, stat.S_IWRITE)
os.remove(name)
shutil.rmtree(path, onerror=del_rw)
Something more robust might check if name is a directory and os.rmdir that - I didn't need it for my case though.
Thanks.
|
msg213867 - (view) |
Author: Jovik (Jovik) |
Date: 2014-03-17 14:09 |
This could be at least part of docs; I found that people tend to avoid shutil.rmtree(...) on Windows because of such issues. Some of them call subprocess("rmdir /S /Q <path>") to get desired behavior.
|
msg217997 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2014-05-06 17:13 |
Ok, so to move this forward we have essentially two proposals:
1) Add a remove_readonly flag
2) Add a doc example which shows how to use the onerror handler to remove a recalcitrant file.
I'm -0.5 on (1) because it feels like Windows-specific clutter; and +0 on (2).
|
msg218021 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-05-06 21:17 |
I'm good with just adding an example to the docs, along the lines of Paul's del_rw. I think it would be better to use a more conservative example though, something like:
def readonly_handler(rm_func, path, exc_info):
if issubclass(exc_info[0], PermissionError) and exc_info[1].winerror == 5:
os.chmod(path, stat.S_IWRITE)
return rm_func(path)
raise exc_info[1]
|
msg218023 - (view) |
Author: Paul Moore (paul.moore) * |
Date: 2014-05-06 21:45 |
Checking the exact error could be a bit fragile. I have a feeling I recently saw an issue somewhere with code that stopped working on Python 3.4 because the precise error raised for a read-only file changed. I don't recall where the issue was, unfortunately.
It's also worth noting that trapping too broad a set of errors won't actually matter much, because the retry will simply fail again if the actual problem wasn't a read-only file...
|
msg218044 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2014-05-07 10:17 |
The attached patch adds an example to the shutil documentation showing how to use an onerror handler to reattempt the removal of a read-only file. It's deliberately low-tech and simply removes the attribute and retries. If there's some other obstacle, it will continue to fail as it would have done in any case.
|
msg218051 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-05-07 13:50 |
Fair point, Paul.
Patch looks good to me, Tim, barring a couple of nits pointed out on Rietveld.
|
msg218052 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2014-05-07 13:59 |
Thanks, Zach. Updated patch.
|
msg218053 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-05-07 14:00 |
LGTM!
|
msg218054 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2014-05-07 14:05 |
Thanks. I'll hold off pushing until I've had a chance to run it on a
Unix system. I'm not 100% whether it will operate in the same way there.
|
msg218063 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-05-07 17:08 |
New changeset 31d63ea5dffa by Tim Golden in branch 'default':
Issue19643 Add an example of shutil.rmtree which shows how to cope with readonly files on Windows
http://hg.python.org/cpython/rev/31d63ea5dffa
New changeset a7560c8f38ee by Tim Golden in branch 'default':
Issue19643 Fix whitespace
http://hg.python.org/cpython/rev/a7560c8f38ee
|
msg229538 - (view) |
Author: Spencer Judge (sushisource) |
Date: 2014-10-16 16:58 |
Although this is closed, I stumbled across it while looking to see if this behavior had changed at all recently, and I have a suggestion I think might work.
How about we take Tim's example error function which was added to the docs, and it's bound to something like shutil.REMOVE_WINDOWS_READONLY, so it can be used in the following way (probably with a better name):
shutil.rmtree("C:/readonlyfilesinhere", onerror=shutil.REMOVE_WINDOWS_READONLY)
Good idea? Too weird?
|
msg229541 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-10-16 17:33 |
I think it is an interesting idea. Probably worth opening a new enhancement request with the suggestion.
|
msg229542 - (view) |
Author: Paul Moore (paul.moore) * |
Date: 2014-10-16 17:37 |
Not a bad idea. I would want the implementation to remain in the documentation as well, as code that wants to be portable back to earlier versions of Python will need it. But for code that targets Python 3.5+ only, having it available "out of the box" is a nice addition.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:53 | admin | set | github: 63842 |
2014-10-16 17:37:23 | paul.moore | set | messages:
+ msg229542 |
2014-10-16 17:33:55 | r.david.murray | set | messages:
+ msg229541 |
2014-10-16 16:58:43 | sushisource | set | nosy:
+ sushisource messages:
+ msg229538
|
2014-07-22 20:36:43 | r.david.murray | link | issue22040 superseder |
2014-05-07 17:34:58 | tim.golden | set | status: open -> closed resolution: fixed stage: commit review -> resolved |
2014-05-07 17:08:14 | python-dev | set | nosy:
+ python-dev messages:
+ msg218063
|
2014-05-07 14:05:42 | tim.golden | set | messages:
+ msg218054 |
2014-05-07 14:00:24 | zach.ware | set | messages:
+ msg218053 stage: commit review |
2014-05-07 13:59:30 | tim.golden | set | files:
+ issue19643-doc.2.patch assignee: tim.golden messages:
+ msg218052
|
2014-05-07 13:50:36 | zach.ware | set | messages:
+ msg218051 |
2014-05-07 10:17:21 | tim.golden | set | files:
+ issue19643-doc.patch keywords:
+ patch |
2014-05-07 10:17:08 | tim.golden | set | messages:
+ msg218044 |
2014-05-06 21:45:01 | paul.moore | set | messages:
+ msg218023 |
2014-05-06 21:17:30 | zach.ware | set | messages:
+ msg218021 |
2014-05-06 17:13:04 | tim.golden | set | messages:
+ msg217997 |
2014-03-17 14:09:28 | Jovik | set | nosy:
+ Jovik messages:
+ msg213867
|
2014-01-21 16:13:26 | paul.moore | set | messages:
+ msg208662 |
2014-01-21 15:50:16 | r.david.murray | set | messages:
+ msg208657 |
2014-01-21 15:29:44 | paul.moore | set | messages:
+ msg208654 |
2014-01-21 15:17:43 | r.david.murray | set | messages:
+ msg208651 |
2014-01-21 14:44:05 | paul.moore | set | nosy:
+ paul.moore messages:
+ msg208647
|
2013-11-18 19:33:24 | zach.ware | set | messages:
+ msg203316 |
2013-11-18 19:32:45 | tim.golden | set | messages:
+ msg203315 |
2013-11-18 19:24:13 | r.david.murray | set | messages:
+ msg203314 |
2013-11-18 19:06:40 | zach.ware | set | messages:
+ msg203313 |
2013-11-18 16:54:24 | ivan.radic | set | messages:
+ msg203308 |
2013-11-18 15:34:44 | r.david.murray | set | messages:
+ msg203301 |
2013-11-18 15:08:59 | tim.golden | set | messages:
+ msg203298 |
2013-11-18 14:53:21 | r.david.murray | set | versions:
+ Python 3.5, - Python 2.7, Python 3.3 nosy:
+ tim.golden, r.david.murray, zach.ware
messages:
+ msg203297
components:
+ Library (Lib), Windows, - IO type: behavior -> enhancement |
2013-11-18 12:11:11 | ivan.radic | create | |