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

bpo-38811: Check for presence of os.link method in pathlib (v2) #17225

Merged
merged 1 commit into from Dec 16, 2019

Conversation

@tohojo
Copy link
Contributor

tohojo commented Nov 18, 2019

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

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>
@the-knights-who-say-ni

This comment has been minimized.

Copy link

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 Missing

Our records indicate the following people have not signed the CLA:

@tohojo

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@tohojo

This comment has been minimized.

Copy link
Contributor Author

tohojo commented Nov 18, 2019

Still working on the CLA... expect to hear back from corporate legal today.

@tohojo

This comment has been minimized.

Copy link
Contributor Author

tohojo commented Nov 18, 2019

@vstinner could you have the build-bot take a look, please? :)

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Nov 18, 2019

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.

@tohojo

This comment has been minimized.

Copy link
Contributor Author

tohojo commented Nov 19, 2019

@vstinner good to go :)

@tohojo

This comment has been minimized.

Copy link
Contributor Author

tohojo commented Nov 26, 2019

Ping? @vstinner ?

@tohojo

This comment has been minimized.

Copy link
Contributor Author

tohojo commented Dec 16, 2019

Could someone please merge this? It's been sitting here for four weeks now... :/

@vstinner or @serhiy-storchaka ?

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Dec 16, 2019

I reverted the first attempt PR #17170 because the CLA was not signed and the test failed on the Windows 7 buildbot:


ERROR: test_symlink_to_not_implemented (test.test_pathlib.PathTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_pathlib.py", line 2031, in test_symlink_to_not_implemented
    link.symlink_to(target)
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\pathlib.py", line 1395, in symlink_to
    self._accessor.symlink(target, self, target_is_directory)
OSError: [WinError 1314] A required privilege is not held by the client: 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\test_python_worker_2032\\@test_2032_tmp\\fileA' -> 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\test_python_worker_2032\\@test_2032_tmp\\dirA\\linkAA'

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 @unittest.skipIf(support.can_symlink() to @unittest.skipIf(pathlib.supports_symlinks).

support.can_symlink() creates a symbolic test and checks for error. It only returns if os.symlink() is available and works. It returns False in case of error.

pathlib.supports_symlinks is true on non-Windows platforms. On Windows, it's only true if sys.getwindowsversion()[:2] >= (6, 0) which checks for Windows Vista and newer. It should be true on Windows 7.

So I understand that the test is now only run on Windows XP and older, since pathlib.supports_symlinks is only false on very old unsupported Windows versions. Am I right? At least, it looks consistent with the pathlib code:

    if nt:
        if supports_symlinks:
            symlink = os.symlink
        else:
            def symlink(a, b, target_is_directory):
                raise NotImplementedError("symlink() not available on this system")
    else:
        # Under POSIX, os.symlink() takes two args
        @staticmethod
        def symlink(a, b, target_is_directory):
            return os.symlink(a, b)

Note: The original PR #17170 was approved by 3 persons (@cool-RR, @serhiy-storchaka, @brandtbucher).

@tohojo tohojo force-pushed the tohojo:bpo-38811 branch from ee219a0 to bf22739 Dec 16, 2019
Copy link
Member

vstinner left a comment

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().

@vstinner vstinner merged commit 092435e into python:master Dec 16, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191216.10 succeeded
Details
bedevere/issue-number Issue number 38811 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Dec 16, 2019

Thanks @tohojo for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

miss-islington added a commit to miss-islington/cpython that referenced this pull request 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>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Dec 16, 2019

GH-17626 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Dec 16, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.