classification
Title: shutil fails when copying to NTFS in Linux
Type: Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Val, amaury.forgeotdarc, christian.heimes, draghuram, facundobatista, gvanrossum, ianare
Priority: normal Keywords: patch

Created on 2007-12-03 07:08 by ianare, last changed 2010-01-03 18:30 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
shutil1.py ianare, 2007-12-03 07:08
shutil.tar.gz ianare, 2007-12-04 17:57
shutil.diff ianare, 2007-12-04 22:13
Messages (30)
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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 2, in <module>
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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) (Python triager) 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 <original file>
<changed file>. 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) (Python triager) 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) (Python triager) 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) (Python triager) 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 ;)
History
Date User Action Args
2010-01-03 18:30:45benjamin.petersonsetstatus: open -> closed
resolution: fixed
2010-01-03 17:50:09Valsetnosy: + Val
messages: + msg97171
2008-08-11 17:26:47draghuramsetmessages: + msg71024
2008-06-20 14:26:21draghuramsetmessages: + msg68460
2007-12-05 22:39:55draghuramsetmessages: + msg58233
2007-12-04 22:13:47ianaresetfiles: + shutil.diff
messages: + msg58204
2007-12-04 21:10:50draghuramsetmessages: + msg58203
2007-12-04 20:46:18ianaresetmessages: + msg58202
2007-12-04 17:57:53ianaresetfiles: + shutil.tar.gz
messages: + msg58198
2007-12-04 17:29:25draghuramsetmessages: + msg58197
2007-12-04 17:12:21ianaresetmessages: + msg58195
2007-12-04 16:53:36ianaresetmessages: + msg58192
2007-12-03 23:49:53gvanrossumsetmessages: + msg58167
2007-12-03 23:39:16ianaresetmessages: + msg58166
2007-12-03 23:22:46draghuramsetmessages: + msg58165
2007-12-03 23:18:59gvanrossumsetmessages: + msg58164
2007-12-03 22:25:22draghuramsetmessages: + msg58162
2007-12-03 22:13:15gvanrossumsetmessages: + msg58161
2007-12-03 22:00:19ianaresetmessages: + msg58160
2007-12-03 21:45:05gvanrossumsetstatus: closed -> open
resolution: rejected -> (no value)
messages: + msg58159
2007-12-03 21:36:24ianaresetmessages: + msg58158
2007-12-03 19:31:09gvanrossumsetnosy: + gvanrossum
messages: + msg58147
2007-12-03 19:23:43facundobatistasetmessages: + msg58146
2007-12-03 19:15:41ianaresetmessages: + msg58142
2007-12-03 17:59:52facundobatistasetstatus: open -> closed
resolution: rejected
messages: + msg58136
2007-12-03 17:53:32ianaresetmessages: + msg58135
2007-12-03 15:49:36amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg58129
2007-12-03 15:33:36draghuramsetnosy: + draghuram
messages: + msg58128
2007-12-03 13:34:37facundobatistasetnosy: + facundobatista
messages: + msg58123
2007-12-03 12:35:08christian.heimessetpriority: normal
keywords: + patch
messages: + msg58117
nosy: + christian.heimes
versions: + Python 2.6, Python 3.0
2007-12-03 07:08:50ianarecreate