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

pathlib.resolve(strict=False) only includes first child #74363

Closed
mshuffett mannequin opened this issue Apr 26, 2017 · 26 comments
Closed

pathlib.resolve(strict=False) only includes first child #74363

mshuffett mannequin opened this issue Apr 26, 2017 · 26 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mshuffett
Copy link
Mannequin

mshuffett mannequin commented Apr 26, 2017

BPO 30177
Nosy @pitrou, @larryhastings, @ned-deily, @zware, @serhiy-storchaka, @eryksun, @zooba, @seirl, @zopieux, @richardcooper
PRs
  • bpo-30177: Fix misleading description of path.resolve() #1649
  • bpo-30177: pathlib: include the full path in resolve(strict=False) #1893
  • [3.6] bpo-30177: pathlib: include the full path in resolve(strict=False) #1985
  • [3.6] bpo-30177: add NEWS entry #2134
  • bpo-30177: add NEWS entry #2135
  • 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 2017-06-07.17:20:32.608>
    created_at = <Date 2017-04-26.16:19:45.264>
    labels = ['3.7', 'type-bug', 'library']
    title = 'pathlib.resolve(strict=False) only includes first child'
    updated_at = <Date 2017-06-12.16:40:16.197>
    user = 'https://bugs.python.org/mshuffett'

    bugs.python.org fields:

    activity = <Date 2017-06-12.16:40:16.197>
    actor = 'ned.deily'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-06-07.17:20:32.608>
    closer = 'steve.dower'
    components = ['Library (Lib)']
    creation = <Date 2017-04-26.16:19:45.264>
    creator = 'mshuffett'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30177
    keywords = []
    message_count = 26.0
    messages = ['292369', '292623', '294647', '294855', '294857', '294859', '294861', '294862', '294863', '294864', '294877', '294879', '294880', '294883', '294889', '294890', '294972', '295206', '295304', '295345', '295346', '295349', '295360', '295361', '295801', '295802']
    nosy_count = 11.0
    nosy_names = ['pitrou', 'larry', 'ned.deily', 'zach.ware', 'serhiy.storchaka', 'eryksun', 'steve.dower', 'antoine.pietri', 'mshuffett', 'zopieux', 'richardc']
    pr_nums = ['1649', '1893', '1985', '2134', '2135']
    priority = 'critical'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30177'
    versions = ['Python 3.6', 'Python 3.7']

    @mshuffett
    Copy link
    Mannequin Author

    mshuffett mannequin commented Apr 26, 2017

    According to the documentation https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve
    If strict is False, the path is resolved as far as possible and any remainder is appended without checking whether it exists.

    The current behavior is not consistent with this, and only appends the first remainder.

    For example:
    If we have an empty '/tmp' directory
    Path('/tmp/foo').resolve() and Path('/tmp/foo/bar').resolve() both result in Path('/tmp/foo') but Path('/tmp/foo/bar').resolve() should result in Path('/tmp/foo/bar')

    @mshuffett mshuffett mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 26, 2017
    @seirl
    Copy link
    Mannequin

    seirl mannequin commented Apr 30, 2017

    I can reproduce this bug. This behavior is really confusing.

    @richardcooper
    Copy link
    Mannequin

    richardcooper mannequin commented May 28, 2017

    Pull Request (PR 1649) treats this as a documentation problem. I would argue that the documentation is correct and this is a bug in the code.

    The strict flag was added as a result of bpo-19717. The decision on what to do when strict=False seems to come in https://mail.python.org/pipermail/python-ideas/2016-September/042203.html where Guido says:

    "I would prefer it if Path.resolve() resolved symlinks until it hits
    something that doesn't exist and then just keep the rest of the path
    unchanged."

    The documented behaviour also seems much more useful than the current behaviour.

    @zooba
    Copy link
    Member

    zooba commented May 31, 2017

    This is definitely a bug if it really behaves as described.

    PRs for fixing the functionality are welcome - I'm inclined to say fix it in 3.6 as well, but it might be too big a behavioural change if people are already working around it.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented May 31, 2017

    In Windows/Python 3.6, the behavior matches the documented one, so this could actually be an important silent bug when executing a code wrote for Windows in another OS. I'd argue to fix it in 3.6 too.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented May 31, 2017

    By the way, I'm pasting what I said on the PR here:

    For what it's worth, this bug was the cause of an important downtime in our organization, because someone needed to normalize a path that was later passed to a .mkdir(parents=True), and it was, in some cases, silently doing the mkdir of a wrong path instead of creating all the parents.

    @pitrou
    Copy link
    Member

    pitrou commented May 31, 2017

    This is definitely a bug and I think the fixes should be backported.

    @zooba
    Copy link
    Member

    zooba commented May 31, 2017

    Ah, didn't catch that it doesn't occur on Windows - that explains why I've never seen it before.

    Yes, definitely fix and backport. Adding the RMs in case they want to delay any upcoming releases to get the fix.

    @zopieux
    Copy link
    Mannequin

    zopieux mannequin commented May 31, 2017

    I agree bpo-30177 is not a suitable fix for this issue, as it fixes the doc instead of fixing the actual underlying bug in the function behavior.

    This bug can lead to files being added to the wrong (parent) directory, which is quite critical.

    @ned-deily
    Copy link
    Member

    We have less than 2 weeks until cutoff for 3.6.2 so it would be great if someone would take this on.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented May 31, 2017

    I'm on it.

    @zooba
    Copy link
    Member

    zooba commented May 31, 2017

    The initial fix should be easy:

    --- a/Lib/pathlib.py
    +++ b/Lib/pathlib.py
    @@ -329,8 +329,6 @@ class _PosixFlavour(_Flavour):
                         if e.errno != EINVAL:
                             if strict:
                                 raise
    -                        else:
    -                            return newpath
                         # Not a symlink
                         path = newpath

    However, the trick is going to be in the tests, which are shared between POSIX and Windows - and are apparently passing on Windows right now. I'm not entirely sure why that is, but it may not be as simple here as "works on POSIX".

    @zooba
    Copy link
    Member

    zooba commented May 31, 2017

    Ah, looks like they require symlinks for the whole test, which means you need to be admin when running them on Windows.

    Zach - do we have any buildbots running as admin for symlink tests?

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented May 31, 2017

    I added a fix for the tests and the code.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jun 1, 2017

    Ah, looks like they require symlinks for the whole test,
    which means you need to be admin when running them on Windows.

    The privilege to create symlinks isn't filtered out of a standard user's token. Are there any buildbots already running as a standard user? In that case it may be simpler to modify the user's rights via secpol.msc: Local Policies -> User Rights Assignment -> Create symbolic links.

    Support could also be added for the new feature in Windows 10 to allow unprivileged creation of symlinks when the system is in developer mode and the flag SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE 1 is used. A keyword-only option to use this flag could be backported to 3.6 and enabled by default in 3.7.

    @zware
    Copy link
    Member

    zware commented Jun 1, 2017

    do we have any buildbots running as admin for symlink tests?

    No, as far as I know. I just took Eryk's suggestion and gave the buildslave user on my Windows 8.1 bot rights to create symbolic links, though. It's now rebooting after updates, we'll see how it does shortly.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented Jun 1, 2017

    So, I asked a friend to check again with a more recent Python version on Windows and he can't reproduce the documented behavior, so the bug seems to also be present on Windows. My patch doesn't address that for now, which explains why the build fails (and why it was working before).

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented Jun 5, 2017

    I updated the PR to fix the Windows part of the issue thanks to Zachary who gave me access to a Windows machine.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented Jun 6, 2017

    The code has been reviewed by (the other) Antoine, I guess there is now everything needed to merge it?

    @zooba
    Copy link
    Member

    zooba commented Jun 7, 2017

    New changeset add98eb by Steve Dower (Antoine Pietri) in branch 'master':
    bpo-30177: pathlib: include the full path in resolve(strict=False) (bpo-1893)
    add98eb

    @zooba
    Copy link
    Member

    zooba commented Jun 7, 2017

    As usual, I can easily hit merge but may not be able to get to the backport immediately. Someone else can feel free to cherrypick and submit the PRs and I'll hit merge on them.

    We should also watch the buildbots for failures though before backporting. Particularly in this area, they should have better coverage than the PR checks.

    @seirl
    Copy link
    Mannequin

    seirl mannequin commented Jun 7, 2017

    The only backport was for 3.6 as the previous versions don't have the strict= parameter. PR submitted here: #1985

    @zooba
    Copy link
    Member

    zooba commented Jun 7, 2017

    New changeset ceabf9a by Steve Dower (Antoine Pietri) in branch '3.6':
    bpo-30177: pathlib: include the full path in resolve(strict=False) (bpo-1893) (bpo-1985)
    ceabf9a

    @zooba
    Copy link
    Member

    zooba commented Jun 7, 2017

    Good point about not needing 3.5.

    Buildbots were clean, so I merged it. Thanks Antoine!

    @zooba zooba closed this as completed Jun 7, 2017
    @ned-deily
    Copy link
    Member

    New changeset 8399a17 by Ned Deily (Antoine Pietri) in branch '3.6':
    [3.6] bpo-30177: add NEWS entry (bpo-2134)
    8399a17

    @ned-deily
    Copy link
    Member

    New changeset a77a35d by Ned Deily (Antoine Pietri) in branch 'master':
    bpo-30177: add NEWS entry (bpo-2135)
    a77a35d

    @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
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants