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

Add symlink support to shutil functions #56924

Closed
akheron opened this issue Aug 9, 2011 · 18 comments
Closed

Add symlink support to shutil functions #56924

akheron opened this issue Aug 9, 2011 · 18 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@akheron
Copy link
Member

akheron commented Aug 9, 2011

BPO 12715
Nosy @pitrou, @vstinner, @tarekziade, @merwok, @socketpair, @akheron, @hynek
Files
  • e126ceae5ba9.diff
  • c92c4d936255.diff
  • 4832bf7aa028.diff
  • d9212bb67f64.diff
  • 23e6204efe20.diff
  • 8330f2045f4d.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 2011-12-29.17:55:47.223>
    created_at = <Date 2011-08-09.12:06:22.071>
    labels = ['type-feature', 'library']
    title = 'Add symlink support to shutil functions'
    updated_at = <Date 2011-12-29.17:55:47.222>
    user = 'https://github.com/akheron'

    bugs.python.org fields:

    activity = <Date 2011-12-29.17:55:47.222>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-12-29.17:55:47.223>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2011-08-09.12:06:22.071>
    creator = 'petri.lehtinen'
    dependencies = []
    files = ['23028', '24067', '24069', '24070', '24099', '24102']
    hgrepos = ['63']
    issue_num = 12715
    keywords = ['patch']
    message_count = 18.0
    messages = ['141813', '141817', '141818', '141819', '141821', '141876', '141966', '142608', '142890', '149986', '149987', '150014', '150027', '150311', '150326', '150333', '150338', '150339']
    nosy_count = 9.0
    nosy_names = ['pitrou', 'vstinner', 'tarek', 'eric.araujo', 'neologix', 'socketpair', 'python-dev', 'petri.lehtinen', 'hynek']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue12715'
    versions = ['Python 3.3']

    @akheron
    Copy link
    Member Author

    akheron commented Aug 9, 2011

    The shutil.copytree() function aknowledges symlinks when given
    the symlink=True argument. Other shutil functions should do this
    too:

    copymode(src, dst, symlinks=True)
       If both src and dst are symlinks, copy the source symlink mode
       to the destination symlink. If only one of them is a symlink,
       dereference it normally.
    
    copystat(src, dst, symlinks=True)
       If both src and dst are symlinks, copy the statinfo of the
       source symlink to the destination symlink. If only one of them
       is a symlink, dereference it normally.
    
    copy(src, dst, symlinks=True)
       If the src argument is a symlink, copy the source symlink
       instead of dereferencing it.

    Problem: What if dst is a symlink to a directory? Should it be
    replaced or should the symlink copied inside that directory?

    copy2(src, dst, symlinks=True)
       Like copy(src, dst, symlinks=True), but copy the metadata too.
       The same problem applies.

    @akheron akheron added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 9, 2011
    @akheron
    Copy link
    Member Author

    akheron commented Aug 9, 2011

    We could also have symlinks='require' to have copymode() and copystat() raise an error unless both src and dst are symlinks.

    @hynek
    Copy link
    Member

    hynek commented Aug 9, 2011

    Additionally copyfile() might be fixed to understand symlinks=True too.

    Currently working on a patch for all 5 of them.

    @merwok
    Copy link
    Member

    merwok commented Aug 9, 2011

    Problem: What if dst is a symlink to a directory? Should it be
    replaced or should the symlink copied inside that directory?

    Doing what UNIX’ cp program does would be best here.

    @akheron
    Copy link
    Member Author

    akheron commented Aug 9, 2011

    Doing what UNIX’ cp program does would be best here.

    $ cp --version
    cp (GNU coreutils) 8.5
    
    $ touch file
    $ ln -s file link_to_file 
    $ mkdir directory
    $ ln -s directory link_to_directory
    $ cp -a link_to_file link_to_directory
    $ ls -l directory
    lrwxrwxrwx 1 user user 4 2011-08-09 17:46 link_to_file -> file

    So at least cp from GNU coreutils seems to copy the link into the directory, dereferencing the dst link.

    @hynek
    Copy link
    Member

    hynek commented Aug 10, 2011

    JFTR, my implementation is ready, but I can't/don't want to start writing tests, as long as bpo-12721 is open.

    @merwok
    Copy link
    Member

    merwok commented Aug 12, 2011

    Other reports related to shutil and symlinks: bpo-9993, bpo-4489, bpo-12461.

    @hynek
    Copy link
    Member

    hynek commented Aug 21, 2011

    We have an edge case when the caller calls copymode with symlinks=True on an OS that doesn't have lchmod (ie any other than FreeBSD).

    Should it be a nop (as in copystat), use chmod or raise an Exception?

    I Personally (and some people on #python-dev) believe it should raise an Exception as the caller made the effort to say that (s)he wants the link _not_ to be followed and it's just one operation (contrary to copystat that does several things).

    @hynek
    Copy link
    Member

    hynek commented Aug 24, 2011

    What started as a 2 lines fix, grew to a patch of ~400 lines. :)

    It's mostly tests though:

    Lib/shutil.py | 102 +++++++++++++++++++++++++++++----------
    Lib/test/test_shutil.py | 220 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
    Misc/ACKS | 1 +
    3 files changed, 297 insertions(+), 26 deletions(-)

    I've AFAIS implemented the desired behavior and hope for feedback/bug reports.

    It's tested on Linux & FreeBSD 8.2 and applies cleanly against default/tip.

    Please let me know if I've done something stupid.

    @hynek
    Copy link
    Member

    hynek commented Dec 21, 2011

    I have updated the patch to latest default/tip and would appreciate a review.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 21, 2011

    I've done a review at http://bugs.python.org/review/12715

    @hynek
    Copy link
    Member

    hynek commented Dec 21, 2011

    Thanks to Antoine & Charles-François for their reviews! I think I've incorporated all desired changes.

    Please let me know if I have missed something, meanwhile I'll tackle the docs.

    @hynek
    Copy link
    Member

    hynek commented Dec 21, 2011

    The patch contains also the necessary changes for the docs now.

    Please let me know, if I can do anything else.

    @hynek
    Copy link
    Member

    hynek commented Dec 28, 2011

    Added another patch fixing the issues pointed out by Antoine and Charles-François.

    @hynek
    Copy link
    Member

    hynek commented Dec 29, 2011

    Added new patch with protection for the remaining UF_NODUMPs in the test case. All raised issues should be fixed now. :)

    @pitrou
    Copy link
    Member

    pitrou commented Dec 29, 2011

    I've checked the latest patch to work fine under Windows 7 (and Linux, of course).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 29, 2011

    New changeset cf57ef65bcd0 by Antoine Pitrou in branch 'default':
    Issue bpo-12715: Add an optional symlinks argument to shutil functions (copyfile, copymode, copystat, copy, copy2).
    http://hg.python.org/cpython/rev/cf57ef65bcd0

    @pitrou
    Copy link
    Member

    pitrou commented Dec 29, 2011

    Patch is now applied to 3.3, thank you for your patience :)

    @pitrou pitrou closed this as completed Dec 29, 2011
    @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 type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants