Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shutil fails when copying to NTFS in Linux #45886

Closed
ianare mannequin opened this issue Dec 3, 2007 · 30 comments
Closed

shutil fails when copying to NTFS in Linux #45886

ianare mannequin opened this issue Dec 3, 2007 · 30 comments
Labels
stdlib Python modules in the Lib dir

Comments

@ianare
Copy link
Mannequin

ianare mannequin commented Dec 3, 2007

BPO 1545
Nosy @gvanrossum, @facundobatista, @amauryfa, @tiran
Files
  • shutil1.py
  • shutil.tar.gz
  • shutil.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2010-01-03.18:30:45.144>
    created_at = <Date 2007-12-03.07:08:50.163>
    labels = ['library']
    title = 'shutil fails when copying to NTFS in Linux'
    updated_at = <Date 2010-01-03.18:30:45.143>
    user = 'https://bugs.python.org/ianare'

    bugs.python.org fields:

    activity = <Date 2010-01-03.18:30:45.143>
    actor = 'benjamin.peterson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-01-03.18:30:45.144>
    closer = 'benjamin.peterson'
    components = ['Library (Lib)']
    creation = <Date 2007-12-03.07:08:50.163>
    creator = 'ianare'
    dependencies = []
    files = ['8859', '8875', '8876']
    hgrepos = []
    issue_num = 1545
    keywords = ['patch']
    message_count = 30.0
    messages = ['58112', '58117', '58123', '58128', '58129', '58135', '58136', '58142', '58146', '58147', '58158', '58159', '58160', '58161', '58162', '58164', '58165', '58166', '58167', '58192', '58195', '58197', '58198', '58202', '58203', '58204', '58233', '68460', '71024', '97171']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'facundobatista', 'amaury.forgeotdarc', 'draghuram', 'christian.heimes', 'ianare', 'Val']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1545'
    versions = ['Python 2.6', 'Python 2.5', 'Python 3.0']

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    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

    @ianare ianare mannequin added the stdlib Python modules in the Lib dir label Dec 3, 2007
    @tiran
    Copy link
    Member

    tiran commented Dec 3, 2007

    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

    @facundobatista
    Copy link
    Member

    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?

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 3, 2007

    I agree with Facundo that it is not good to hide the error. It should be
    up to the caller to ignore the error.

    @amauryfa
    Copy link
    Member

    amauryfa commented Dec 3, 2007

    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.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    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.

    @facundobatista
    Copy link
    Member

    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.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    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)

    @facundobatista
    Copy link
    Member

    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!

    @gvanrossum
    Copy link
    Member

    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().

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    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

    @gvanrossum
    Copy link
    Member

    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.

    @gvanrossum gvanrossum reopened this Dec 3, 2007
    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    Yes, it is a sub-class of OSError. So then only catching OSError should
    be sufficient? Or am I missing something?

    @gvanrossum
    Copy link
    Member

    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

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 3, 2007

    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

    @gvanrossum
    Copy link
    Member

    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\>


    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 3, 2007

    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 :-)

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 3, 2007

    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

    @gvanrossum
    Copy link
    Member

    Look at the except clause in copytree(); it does a bit more,
    collecting the errors and raising them later.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 4, 2007

    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.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 4, 2007

    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)))

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 4, 2007

    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.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 4, 2007

    Sure thing, here it is.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 4, 2007

    hmm, marked me as spam ... the tar.gz posted last message contains the
    original shutil, a patched version, and the diff

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 4, 2007

    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.

    @ianare
    Copy link
    Mannequin Author

    ianare mannequin commented Dec 4, 2007

    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?

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Dec 5, 2007

    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?

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Jun 20, 2008

    I have submitted a patch (http://codereview.appspot.com/2384) for
    WindowsError issue as it is reported in two other bugs bpo-3134 and bpo-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.

    @draghuram
    Copy link
    Mannequin

    draghuram mannequin commented Aug 11, 2008

    WindowsError issue is now fixed in r65644.

    @Val
    Copy link
    Mannequin

    Val mannequin commented Jan 3, 2010

    I think this one is solved now. I recommend just to put as solved and kill it from the list ;)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants