Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-38811: Check for presence of os.link method in pathlib (v2) #17225
Conversation
Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using os.link) (GH-12990)") introduced a new link_to method in pathlib. However, this makes pathlib crash when the 'os' module is missing a 'link' method. Fix this by checking for the presence of the 'link' method on pathlib module import, and if it's not present, turn it into a runtime error like those emitted when there is no lchmod() or symlink(). Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Nov 18, 2019
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
This comment has been minimized.
This comment has been minimized.
Still working on the CLA... expect to hear back from corporate legal today. |
This comment has been minimized.
This comment has been minimized.
@vstinner could you have the build-bot take a look, please? :) |
This comment has been minimized.
This comment has been minimized.
Custom buildbot builds are special. I prefer to wait until the "CLA signed" label appears on the PR. Ping me when it's the case. At least, ping me when "Contributor Form Received" becomes "Yes" at: https://bugs.python.org/user32968 This part is done manually by a real human :-) It may take a few days. |
This comment has been minimized.
This comment has been minimized.
@vstinner good to go :) |
This comment has been minimized.
This comment has been minimized.
Ping? @vstinner ? |
This comment has been minimized.
This comment has been minimized.
Could someone please merge this? It's been sitting here for four weeks now... :/ |
This comment has been minimized.
This comment has been minimized.
I reverted the first attempt PR #17170 because the CLA was not signed and the test failed on the Windows 7 buildbot:
https://bugs.python.org/issue38811#msg356858 On this PR, the CLA is signed: good, thanks @tohojo. The tests pass on Linux and Windows (the pre-commit CIs are green). I tested manually the PR on my Windows 10 VM: test_pathlib pass there as well. This new PR also changed the condition to run the test from
So I understand that the test is now only run on Windows XP and older, since
Note: The original PR #17170 was approved by 3 persons (@cool-RR, @serhiy-storchaka, @brandtbucher). |
LGTM. I asked @tohojo to revert the os.symlink() change (it was a separated commit). To me, it's really unrelated and I'm not confident that the symlink change was correct, since it looks like dead code. It seems like Python no longer supports platforms without os.symlink(). |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Dec 16, 2019
…17225) Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using os.link) (pythonGH-12990)") introduced a new link_to method in pathlib. However, this makes pathlib crash when the 'os' module is missing a 'link' method. Fix this by checking for the presence of the 'link' method on pathlib module import, and if it's not present, turn it into a runtime error like those emitted when there is no lchmod() or symlink(). Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> (cherry picked from commit 092435e) Co-authored-by: Toke Høiland-Jørgensen <toke@redhat.com>
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Dec 16, 2019
GH-17626 is a backport of this pull request to the 3.8 branch. |
Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using os.link) (GH-12990)") introduced a new link_to method in pathlib. However, this makes pathlib crash when the 'os' module is missing a 'link' method. Fix this by checking for the presence of the 'link' method on pathlib module import, and if it's not present, turn it into a runtime error like those emitted when there is no lchmod() or symlink(). Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> (cherry picked from commit 092435e) Co-authored-by: Toke Høiland-Jørgensen <toke@redhat.com>
tohojo commentedNov 18, 2019
•
edited by bedevere-bot
This is a do-over of #17170, with a symlink test that hopefully doesn't crash on Windows.
Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using
os.link) (GH-12990)") introduced a new link_to method in pathlib. However,
this makes pathlib crash when the 'os' module is missing a 'link' method.
Fix this by checking for the presence of the 'link' method on pathlib
module import, and if it's not present, turn it into a runtime error like
those emitted when there is no lchmod() or symlink().
Also, add a test for the NotImplemented error raised by the corresponding symlink_to method.
https://bugs.python.org/issue38811