| msg58112 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 07:08 |
| When using shutil.copy2 or copytree where the source is on a filesystem that has octal permissions (ie ext3) and the destination is on an NTFS partition mounted rw, the operation fails with OSError: [Errno 1] Operation not permitted I am attaching a version of shutil.py where this has been worked around by calling copystat like so: try: copystat(src, dst) except OSError, (n, str): # can't change stats on NTFS partition even if # OS supports it if n == 1: pass else: raise Error, str |
|
|
| msg58117 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2007-12-03 12:35 |
| Better patch: import errno try: copystat(src, dst) except OSError, err: # can't change stats on NTFS partition even if # OS supports it if err.errno != errno.EPERM: raise |
|
|
| msg58123 - (view) |
Author: Facundo Batista (facundobatista) * |
Date: 2007-12-03 13:34 |
| But is ok to hide the problem? I mean, there *is* an error: the operation is not permitted (even if it's not Python fault), so why to not have the exception? |
|
|
| msg58128 - (view) |
Author: Raghuram Devarakonda (draghuram)  |
Date: 2007-12-03 15:33 |
| I agree with Facundo that it is not good to hide the error. It should be up to the caller to ignore the error. |
|
|
| msg58129 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2007-12-03 15:49 |
| This reminds me of an option in os.walk: """ By default errors from the os.listdir() call are ignored. If optional argument onerror is specified, it should be a function; it will be called with one argument, an OSError instance. It can report the error to continue with the walk, or raise the exception to abort the walk. Note that the filename is available as the filename attribute of the exception object. """ We could do something similar, except that by default, errors are not ignored. |
|
|
| msg58135 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 17:53 |
| I agree, the best would be to have the option of ignoring errors. However, I think there should be a way to differentiate between errors that prevent a file from being copied, and one that only affects permissions. |
|
|
| msg58136 - (view) |
Author: Facundo Batista (facundobatista) * |
Date: 2007-12-03 17:59 |
| os.walk has an error handling mechanism because it goes through a collection of files, so it's nice to have, for example, a function applied if errors appear in some of those files. As this operation is applicable only to one file, this is not useful at all: if you want to catch the error, just do it. Regarding the type of error, note that you can use the mechanism tiran shows below (if err.errno equals errno.EPERM). In any case, this bug is not applicable any more. |
|
|
| msg58142 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 19:15 |
| I must respectfully disagree. This bug also occurs during the copytree command, so it can apply to more than one file. And if using from move then the original file never gets deleted. As far as working around it, it is obviously doable, however this requires distributing my own version of shutil with my application, something which is clearly undesirable. Furthermore, I think that being able to ignore certain errors is very much desirable for this library. (maybe this could be filed under a different report) |
|
|
| msg58146 - (view) |
Author: Facundo Batista (facundobatista) * |
Date: 2007-12-03 19:23 |
| I'm -1 to the library ignore errors without specific indication from the user, specially when it's so easy to do it in the calling code. We agree that copytree could grow an option like the one in os.walk. Feel free to open an issue for it (this discussion went through other path here, that's why I prefer a clean issue; you should refer this one in the text, though). You should first discuss this in the list, also. Thank you! |
|
|
| msg58147 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-03 19:31 |
| There's already an except clause for copystat() that ignores WindowsError in copytree(). If copying this same code into copy2() makes the OP happy I think we can place a similar except clause around the copystat() call in copy2(). |
|
|
| msg58158 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 21:36 |
| The problem with WindowsError is that it is not included in Linux. So if there is an exception, it fails without showing the actual error. See here (ubuntu 7.10 default install): >>> try: print a ... except WindowsError: ... print 'b' ... Traceback (most recent call last): File "", line 2, in NameError: name 'WindowsError' is not defined It should return "NameError: name 'a' is not defined" In my workaround, by placing the OSError before the WindowsError, this problem is taken care of. Let me see if I can figure out something a little better than always ignoring this type of error ... Thanks |
|
|
| msg58159 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-03 21:45 |
| Hm, that means there's a bug in the existing copytree() code too! Can you check whether WindowsError derives from OSError? If it does, your proposal won't fly. |
|
|
| msg58160 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 22:00 |
| Yes, it is a sub-class of OSError. So then only catching OSError should be sufficient? Or am I missing something? |
|
|
| msg58161 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-03 22:13 |
| Not quite, because we only want to ignore WindowsError. Maybe this would work? try: WindowsError except NameError: WindowsError = None try: .... except os.error, err: if WindowsError is not None or not isinstance(err, WindowsError): raise # Pretend we didn't catch it pass # Ignore it |
|
|
| msg58162 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-03 22:25 |
| > try: > .... > except os.error, err: > if WindowsError is not None or not isinstance(err, WindowsError): > raise # Pretend we didn't catch it > pass # Ignore it All the double negations are hurting when I try to understand above conditions. How about (in addition to the WindowsError name check): if WindowsError and isinstance(err, WindowsError): pass # ignore raise |
|
|
| msg58164 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-03 23:18 |
| You're right, my code was wrong. Yours will be be correct if you add "else:" in front of the raise. I also still prefer "WindowsError is not None" over just "WindowsError". On Dec 3, 2007 2:25 PM, Raghuram Devarakonda <report@bugs.python.org> wrote: > > Raghuram Devarakonda added the comment: > > > try: > > .... > > except os.error, err: > > if WindowsError is not None or not isinstance(err, WindowsError): > > raise # Pretend we didn't catch it > > pass # Ignore it > > All the double negations are hurting when I try to understand above > conditions. How about (in addition to the WindowsError name check): > > if WindowsError and isinstance(err, WindowsError): > pass # ignore > > raise > > > __________________________________ > Tracker <report@bugs.python.org> > <http://bugs.python.org/issue1545> > __________________________________ > |
|
|
| msg58165 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-03 23:22 |
| > You're right, my code was wrong. Yours will be be correct if you add > "else:" in front of the raise. I also still prefer "WindowsError is Yeah. I was just about to correct my code when I saw your response. I should not post just before leaving work for home :-) |
|
|
| msg58166 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-03 23:39 |
| Rather than try in copytree and try again in copy2, why not add this to the root of the issue - copystat. Something like this? def copystat(src, dst, ignore_permission_err=False): """Copy all stat info (mode bits, atime and mtime) from src to dst""" st = os.stat(src) mode = stat.S_IMODE(st.st_mode) try: if hasattr(os, 'utime'): os.utime(dst, (st.st_atime, st.st_mtime)) if hasattr(os, 'chmod'): os.chmod(dst, mode) except OSError, err: if WindowsError is not None and isinstance(err, WindowsError): pass elif ignore_permissions_err and err.errno == errno.EPERM: pass else: raise |
|
|
| msg58167 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-03 23:49 |
| Look at the except clause in copytree(); it does a bit more, collecting the errors and raising them later. |
|
|
| msg58192 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-04 16:53 |
| OK I see that now. For what it's worth, I tested the code on win2000, XP, and ubuntu using the shutil.move command on files and folders (so that it uses either copy2 or copytree). Apart from the original bug in ubuntu (copy from ext3 to ntfs-3g) it is fine. I've made further modifications to allow for not showing permission errors. Should I file these under a new report? Many thanks. |
|
|
| msg58195 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-04 17:12 |
| sorry, should have clarified, I tested with this code: copy2 try: copystat(src, dst) except OSError, err: if WindowsError is not None and isinstance(err, WindowsError): pass else: raise copytree try: copystat(src, dst) except OSError, err: if WindowsError is not None and isinstance(err, WindowsError): pass else: errors.extend((src, dst, str(why))) |
|
|
| msg58197 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-04 17:29 |
| The change looks fine as per yesterday's discussion. Can you submit the actual patch? it needs to include check for WindowsError name. BTW, I would put a comment when the exception is being ignored on windows. |
|
|
| msg58198 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-04 17:57 |
| Sure thing, here it is. |
|
|
| msg58202 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-04 20:46 |
| hmm, marked me as spam ... the tar.gz posted last message contains the original shutil, a patched version, and the diff |
|
|
| msg58203 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-04 21:10 |
| Some comments about shutil.diff: 1) This seems to be against 2.5. You would be better off submitting a patch against trunk. In case of bug fixes, it will usually be back ported. This particular change is not a bug fix though. 2) patches should be either context diff or preferably unified diffs. In addition, a patch should be a forward diff (diff . It is best to simply do "svn diff". 3) Now for the actual content of the patch, it must be noted that ignoring WindowsError in copy2 breaks compatibility. Not that there is any reason for any one to be depending on it, but I just thought I would point it out. In any case, it will require slight doc change. Lastly, just to be clear, the original problem reported in this bug would still remain. |
|
|
| msg58204 - (view) |
Author: ianaré (ianare) |
Date: 2007-12-04 22:13 |
| Sorry about that. Here is the output from $ svn diff I've also made modifications to allow to ignore the permission errors (defaults to no) - should I post here or file new report? |
|
|
| msg58233 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-05 22:39 |
| There is a mistake in the patch. The line "if WindowsError is not None and isinstance(err, WindowsError):" in copytree() should use the name 'why' and not 'err'. Unfortunately I can't test on windows, but you didn't get NameError when you tested copytree() on windows? |
|
|
| msg68460 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-06-20 14:26 |
| I have submitted a patch (http://codereview.appspot.com/2384) for WindowsError issue as it is reported in two other bugs #3134 and #2549. I have only tested on Linux so I would appreciate if some one who have access to windows can test it as well. Considering that the fix is simple and more bugs are being opened for the same problem, it is worthwhile to check this in quickly. |
|
|
| msg71024 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-08-11 17:26 |
| WindowsError issue is now fixed in r65644. |
|
|
| msg97171 - (view) |
Author: Valentín (Val) |
Date: 2010-01-03 17:50 |
| I think this one is solved now. I recommend just to put as solved and kill it from the list ;) |
|
|