classification
Title: pathlib.resolve(strict=False) only includes first child
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: antoine.pietri, eryksun, larry, mshuffett, ned.deily, pitrou, richardc, serhiy.storchaka, steve.dower, zach.ware, zopieux
Priority: critical Keywords:

Created on 2017-04-26 16:19 by mshuffett, last changed 2017-06-12 16:40 by ned.deily. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1649 closed Dormouse759, 2017-05-18 12:25
PR 1893 merged python-dev, 2017-05-31 23:40
PR 1985 merged antoine.pietri, 2017-06-07 15:41
PR 2134 merged antoine.pietri, 2017-06-12 16:16
PR 2135 merged antoine.pietri, 2017-06-12 16:33
Messages (26)
msg292369 - (view) Author: Michael Shuffett (mshuffett) Date: 2017-04-26 16:19
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')
msg292623 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-04-30 10:12
I can reproduce this bug. This behavior is really confusing.
msg294647 - (view) Author: Richard Cooper (richardc) Date: 2017-05-28 13:37
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 issue19717. 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.
msg294855 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-05-31 17:23
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.
msg294857 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-05-31 17:26
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.
msg294859 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-05-31 17:33
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.
msg294861 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-31 17:38
This is definitely a bug and I think the fixes should be backported.
msg294862 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-05-31 17:44
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.
msg294863 - (view) Author: zopieux (zopieux) * Date: 2017-05-31 17:48
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.
msg294864 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-05-31 18:22
We have less than 2 weeks until cutoff for 3.6.2 so it would be great if someone would take this on.
msg294877 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-05-31 22:54
I'm on it.
msg294879 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-05-31 23:27
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".
msg294880 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-05-31 23:29
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?
msg294883 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-05-31 23:44
I added a fix for the tests and the code.
msg294889 - (view) Author: Eryk Sun (eryksun) * Date: 2017-06-01 00:42
> 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.

[1]: https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10
msg294890 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2017-06-01 01:07
> 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.
msg294972 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-06-01 21:27
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).
msg295206 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-06-05 17:01
I updated the PR to fix the Windows part of the issue thanks to Zachary who gave me access to a Windows machine.
msg295304 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-06-06 21:19
The code has been reviewed by (the other) Antoine, I guess there is now everything needed to merge it?
msg295345 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-06-07 15:29
New changeset add98eb4fe41baeaa70fbd4ccc020833740609a4 by Steve Dower (Antoine Pietri) in branch 'master':
bpo-30177: pathlib: include the full path in resolve(strict=False) (#1893)
https://github.com/python/cpython/commit/add98eb4fe41baeaa70fbd4ccc020833740609a4
msg295346 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-06-07 15:30
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.
msg295349 - (view) Author: Antoine Pietri (antoine.pietri) * Date: 2017-06-07 15:43
The only backport was for 3.6 as the previous versions don't have the strict= parameter. PR submitted here: https://github.com/python/cpython/pull/1985
msg295360 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-06-07 17:18
New changeset ceabf9acf03f9bbe660d856bff90ecab475ab543 by Steve Dower (Antoine Pietri) in branch '3.6':
bpo-30177: pathlib: include the full path in resolve(strict=False) (#1893) (#1985)
https://github.com/python/cpython/commit/ceabf9acf03f9bbe660d856bff90ecab475ab543
msg295361 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-06-07 17:20
Good point about not needing 3.5.

Buildbots were clean, so I merged it. Thanks Antoine!
msg295801 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-06-12 16:23
New changeset 8399a177de8bfa860a66e96665488c17199cb9d2 by Ned Deily (Antoine Pietri) in branch '3.6':
[3.6] bpo-30177: add NEWS entry (#2134)
https://github.com/python/cpython/commit/8399a177de8bfa860a66e96665488c17199cb9d2
msg295802 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-06-12 16:40
New changeset a77a35d70ba8aed047e63d4d9f7d0554e98d4c4b by Ned Deily (Antoine Pietri) in branch 'master':
bpo-30177: add NEWS entry (#2135)
https://github.com/python/cpython/commit/a77a35d70ba8aed047e63d4d9f7d0554e98d4c4b
History
Date User Action Args
2017-06-12 16:40:16ned.deilysetmessages: + msg295802
2017-06-12 16:33:01antoine.pietrisetpull_requests: + pull_request2189
2017-06-12 16:23:26ned.deilysetmessages: + msg295801
2017-06-12 16:16:40antoine.pietrisetpull_requests: + pull_request2188
2017-06-07 17:20:32steve.dowersetstatus: open -> closed
versions: - Python 3.5
messages: + msg295361

resolution: fixed
stage: backport needed -> resolved
2017-06-07 17:18:58steve.dowersetmessages: + msg295360
2017-06-07 15:43:35antoine.pietrisetmessages: + msg295349
2017-06-07 15:41:30antoine.pietrisetpull_requests: + pull_request2052
2017-06-07 15:30:26steve.dowersetmessages: + msg295346
stage: backport needed
2017-06-07 15:29:20steve.dowersetmessages: + msg295345
2017-06-06 21:19:02antoine.pietrisetmessages: + msg295304
2017-06-05 17:01:40antoine.pietrisetmessages: + msg295206
2017-06-01 21:27:02antoine.pietrisetmessages: + msg294972
2017-06-01 01:07:08zach.waresetmessages: + msg294890
2017-06-01 00:42:45eryksunsetnosy: + eryksun
messages: + msg294889
2017-05-31 23:44:27antoine.pietrisetmessages: + msg294883
2017-05-31 23:40:47python-devsetpull_requests: + pull_request1969
2017-05-31 23:29:29steve.dowersetnosy: + zach.ware
messages: + msg294880
2017-05-31 23:27:08steve.dowersetmessages: + msg294879
2017-05-31 22:54:42antoine.pietrisetmessages: + msg294877
2017-05-31 18:22:01ned.deilysetmessages: + msg294864
2017-05-31 17:48:38zopieuxsetnosy: + zopieux
messages: + msg294863
2017-05-31 17:44:03steve.dowersetpriority: normal -> critical
nosy: + larry, ned.deily
messages: + msg294862

2017-05-31 17:38:34pitrousetmessages: + msg294861
versions: + Python 3.5
2017-05-31 17:33:39antoine.pietrisetmessages: + msg294859
2017-05-31 17:26:12antoine.pietrisetmessages: + msg294857
2017-05-31 17:23:22steve.dowersetnosy: + steve.dower
messages: + msg294855
2017-05-28 13:37:40richardcsetnosy: + richardc
messages: + msg294647
2017-05-18 12:25:13Dormouse759setpull_requests: + pull_request1744
2017-04-30 10:12:58antoine.pietrisetnosy: + antoine.pietri
messages: + msg292623
2017-04-26 17:18:12serhiy.storchakasetnosy: + pitrou, serhiy.storchaka
2017-04-26 16:19:45mshuffettcreate