bpo-30177: pathlib: include the full path in resolve(strict=False) #1893
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
Apparently the fixed testsuite fails on windows, but I no longer have a windows machine to try and understand why. Could someone with this system give a look? |
seirl
force-pushed
the
master
branch
2 times, most recently
from
62fcd90 to
5850899
Compare
|
Thanks to @zware who gave me access to a windows machine, I've been able to make on a fix for Windows too. It passes all the tests. While working on this issue, I think I have found another problem with the way the infinite loops are handled on Windows since 4b1e98b: the documentation states that it should raise a RuntimeError (and it does on POSIX), but on Windows it only returns the path as-is without resolving it. The associated test is only ran for POSIX. We should probably open a new issue for that one, unless I'm missing something in the code? |
| while True: | ||
| try: | ||
| s = self._ext_to_normal(_getfinalpathname(s)) | ||
| except FileNotFoundError: | ||
| previous_s = s | ||
| s = os.path.dirname(s) | ||
| s, tail = os.path.split(s) |
There was a problem hiding this comment.
pathlib never uses os.path.split or os.path.join as they might not return the right results.
There was a problem hiding this comment.
So os.path.split doesn't work but os.path.dirname and os.path.basename can be used?
There was a problem hiding this comment.
Hmm... You're right that it's incoherent. Did you test resolving weird paths such as UNC paths? Because AFAIR that's where the issues can lie.
There was a problem hiding this comment.
The code I'm replacing was already using os.path.dirname so I'm not sure whether the code was already wrong before or if this function works.
There was a problem hiding this comment.
No, I haven't tried myself but I've seen some tests that did things like that for resolve()... I'll check later.
There was a problem hiding this comment.
@pitrou I did some tests and I'm really not sure what the problem is with UNC paths... I think if there is a problem, it won't be introduced by this patch anyway as this code already uses os.path.dirname, so it's probably irrelevant?
Since the goal is to backport that fix, I thought it would be best to keep it as simple as possible.
There was a problem hiding this comment.
Thanks for testing. You're right, if we don't risk a regression then your strategy is ok.
There was a problem hiding this comment.
Though for clarity, I'm fairly sure I introduced the call to dirname without knowing that we should avoid it. But it should be safer than split and join because I don't think those necessarily handle drive/machine names properly while dirname certainly should.
A comment on the implentation: instead of just appending the rest of the path, we do a readlink for all the components, which means we potentially do useless checks if we already know the parent doesn't exist. This simplifies the fix a lot and is coherent with the behavior of
readlink -m:lstat("/a", 0x7fff81db7c80) = -1 ENOENT (No such file or directory)
lstat("/a/b", 0x7fff81db7c80) = -1 ENOENT (No such file or directory)
lstat("/a/b/c", 0x7fff81db7c80) = -1 ENOENT (No such file or directory)
lstat("/a/b/d", 0x7fff81db7c80) = -1 ENOENT (No such file or directory)