Reduce dependency on ctypes when discovering glibc version. #6678

brandtbucher · 2019-07-03T05:47:48Z

Favor os.confstr when possible, falling back on the old ctypes implementation otherwise. Gracefully handle a missing ctypes module.

Closes #6543. Closes #6675. Supersedes and closes #6544.

chrahunt

Our current tests are lacking for this function in particular. We should add a few tests, at least checking:

os.confstr throwing an expected exceptions falls back to ctypes-derived version
ctypes import failure causes None to be returned

src/pip/_internal/utils/glibc.py

chrahunt

Thanks for the update! Just a few small followup comments.

src/pip/_internal/utils/glibc.py

tests/unit/test_utils.py

cjerdonek · 2019-07-07T23:36:41Z

src/pip/_internal/utils/glibc.py

+
+    # This strategy is used in the standard library platform module:
+    # https://github.com/python/cpython/blob/fcf1d003bf4f0100c9d0921ff3d70e1127ca1b71/Lib/platform.py#L175-L183
+


Just a quick note: you don't need any of these three empty lines. Also, the three sentences can follow each other like a paragraph, with the link on its own line at the end.

chrahunt

LGTM. 👍

cjerdonek

A few more comments.

tests/unit/test_utils.py

cjerdonek · 2019-07-08T11:24:38Z

tests/unit/test_utils.py

@@ -701,6 +704,31 @@ def test_manylinux_check_glibc_version(self):
                    # Didn't find the warning we were expecting
                    assert False

+    def test_glibc_version_string_confstr_fail(self, monkeypatch):


How about a successful case, too? In addition, it would probably be good to have at least one successful test of glibc_version_string(), which you also modified. (The code for this test can be basically the same as the one for glibc_version_string_confstr().)

news/6543.bugfix

brandtbucher · 2019-07-11T07:05:36Z

Thanks for the feedback, @cjerdonek! Tests are passing, ready for another review.

brandtbucher · 2019-07-15T19:39:27Z

Is there anything else that this needs before merging? @cjerdonek

pradyunsg · 2019-07-21T12:47:56Z

I'm gonna take the liberty of merging this without waiting on Chris to approve this PR -- if there's any more issues, we can address them in a follow up.

brandtbucher · 2019-07-21T13:52:10Z

Thanks @pradyunsg!

brandtbucher added 5 commits July 2, 2019 10:47

Get glibc version from os instead of ctypes.

129c383

Add news entries.

71915fa

Fall back on ctypes.

d74da18

Clean up diff.

ad4638d

Fall back on ctypes when os.confstr parsing fails.

e27f494

chrahunt suggested changes Jul 4, 2019

View reviewed changes

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

src/pip/_internal/utils/glibc.py Show resolved Hide resolved

cjerdonek reviewed Jul 5, 2019

View reviewed changes

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

brandtbucher added 2 commits July 6, 2019 23:35

Feedback from code review.

2cc9065

Fix test patching.

971e9e1

chrahunt reviewed Jul 7, 2019

View reviewed changes

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

src/pip/_internal/utils/glibc.py Outdated Show resolved Hide resolved

tests/unit/test_utils.py Outdated Show resolved Hide resolved

cjerdonek reviewed Jul 7, 2019

View reviewed changes

Second round of feedback.

9c535d0

chrahunt approved these changes Jul 8, 2019

View reviewed changes

cjerdonek reviewed Jul 8, 2019

View reviewed changes

Third round of feedback.

f09bc7a

pradyunsg approved these changes Jul 20, 2019

View reviewed changes

pradyunsg added the type: enhancement Improvements to functionality label Jul 20, 2019

xavfernandez approved these changes Jul 20, 2019

View reviewed changes

pradyunsg merged commit 8582f7e into pypa:master Jul 21, 2019

chrahunt mentioned this pull request Jul 24, 2019

Port changes to reduce dependencies on ctypes module in packaging.tags. pypa/packaging#171

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 20, 2019

lock bot locked as resolved and limited conversation to collaborators Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce dependency on ctypes when discovering glibc version. #6678

Reduce dependency on ctypes when discovering glibc version. #6678

brandtbucher commented Jul 3, 2019 •

edited by pradyunsg

chrahunt left a comment

chrahunt left a comment

cjerdonek Jul 7, 2019

chrahunt left a comment

cjerdonek left a comment

cjerdonek Jul 8, 2019

brandtbucher commented Jul 11, 2019

brandtbucher commented Jul 15, 2019

pradyunsg commented Jul 21, 2019 •

edited

brandtbucher commented Jul 21, 2019


		# This strategy is used in the standard library platform module:
		# https://github.com/python/cpython/blob/fcf1d003bf4f0100c9d0921ff3d70e1127ca1b71/Lib/platform.py#L175-L183

Reduce dependency on ctypes when discovering glibc version. #6678

Reduce dependency on ctypes when discovering glibc version. #6678

Conversation

brandtbucher commented Jul 3, 2019 • edited by pradyunsg

chrahunt left a comment

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

cjerdonek Jul 7, 2019

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

cjerdonek left a comment

Choose a reason for hiding this comment

cjerdonek Jul 8, 2019

Choose a reason for hiding this comment

brandtbucher commented Jul 11, 2019

brandtbucher commented Jul 15, 2019

pradyunsg commented Jul 21, 2019 • edited

brandtbucher commented Jul 21, 2019

brandtbucher commented Jul 3, 2019 •

edited by pradyunsg

pradyunsg commented Jul 21, 2019 •

edited