Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

gh-90548: Make musl test skips smarter (fixes Alpine errors) #131313

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Mar 19, 2025
Merged

gh-90548: Make musl test skips smarter (fixes Alpine errors) #131313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2348711
Make musl test skips smarter (fixes Alpine errors)
bitdancer Mar 15, 2025
35960c1
Platform tests and bug fixes.
bitdancer Mar 15, 2025
417143e
Add docs, including some existing is_xxx's.
bitdancer Mar 15, 2025
ef36eb2
Address vstinner review comments.
bitdancer Mar 17, 2025
909ea3b
Test some version number variants we accept.
bitdancer Mar 17, 2025
4e76d78
Add news item about libc_ver enhancement.
bitdancer Mar 17, 2025
83add5d
Fix 'in' phrase logic.
bitdancer Mar 18, 2025
4d6aa92
Prettify re expression using re.VERBOSE.
bitdancer Mar 18, 2025
111e2ec
Use more verbose style rather than :=
bitdancer Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Platform tests and bug fixes. 
In adding tests for the new platform code I found a bug in the old code:
if a valid version is passed for version and it is greater than the
version found for an so *and* there is no glibc version, then the
version from the argument was returned.  The code changes here fix
that, as well as fixing my own broken additions.
  • Loading branch information
@bitdancer
bitdancer committed Mar 15, 2025
commit 35960c121b64da4d751a66a0c12a7976403b9dfb
36 changes: 19 additions & 17 deletions 36 Lib/platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,26 +190,27 @@ def libc_ver(executable=None, lib='', version='', chunksize=16384):
return lib, version

libc_search = re.compile(
b'(__libc_init)'
b'|'
b'(GLIBC_([0-9.]+))'
b'|'
br'(__libc_init)'
br'|'
br'(GLIBC_([0-9.]+))'
br'|'
br'(libc(_\w+)?\.so(?:\.(\d[0-9.]*))?)'
b'|'
b'(musl-([0-9.]+))'
b'',
br'|'
br'(musl-([0-9.]+))'
br'',
picnixz marked this conversation as resolved.
Show resolved Hide resolved
re.ASCII)

V = _comparable_version
# We use os.path.realpath()
# here to work around problems with Cygwin not being
# able to open symlinks for reading
executable = os.path.realpath(executable)
ver = None
with open(executable, 'rb') as f:
binary = f.read(chunksize)
pos = 0
while pos < len(binary):
if b'libc' in binary or b'GLIBC' in binary:
if b'libc' in binary or b'GLIBC' or 'musl' in binary:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if b'libc' in binary or b'GLIBC' or 'musl' in binary:
if b'libc' in binary or b'GLIBC' or b'musl' in binary:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a nasty one. The actual fix is b'GLIBC in binary or b'musl' in binary`. That's why it passed without the b...it just ran slower ;) (A lot slower, somewhat to my surprise.)

m = libc_search.search(binary, pos)
else:
m = None
Expand All @@ -229,21 +230,22 @@ def libc_ver(executable=None, lib='', version='', chunksize=16384):
elif glibc:
if lib != 'glibc':
lib = 'glibc'
version = glibcversion
elif V(glibcversion) > V(version):
version = glibcversion
ver = glibcversion
elif V(glibcversion) > V(ver):
ver = glibcversion
elif so:
if lib != 'glibc':
lib = 'libc'
if soversion and (not version or V(soversion) > V(version)):
version = soversion
if threads and version[-len(threads):] != threads:
version = version + threads
if soversion and (not ver or V(soversion) > V(ver)):
ver = soversion
if threads and ver[-len(threads):] != threads:
ver = ver + threads
elif musl:
lib = 'musl'
version = muslversion
if not ver or V(muslversion) > V(ver):
ver = muslversion
pos = m.end()
return lib, version
return lib, version if ver is None else ver

def _norm_version(version, build=''):

Expand Down
32 changes: 24 additions & 8 deletions 32 Lib/test/test_platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,7 @@ def test_libc_ver(self):
(b'GLIBC_2.9', ('glibc', '2.9')),
(b'libc.so.1.2.5', ('libc', '1.2.5')),
(b'libc_pthread.so.1.2.5', ('libc', '1.2.5_pthread')),
(b'/aports/main/musl/src/musl-1.2.5', ('musl', '1.2.5')),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests on version with 2 numbers and 4 numbers? Like /aports/main/musl/src/musl-1.2 and /aports/main/musl/src/musl-1.2.5.1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but I don't believe musl will ever have such release numbers, so is it worth doing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing a binary file using a regular expression is a strange thing for me. So I prefer to have tests to make sure that the implementation is reliable. The question is more if the regex will match versions with 2 members (x.y) or 4 members (x.y.z.a), or if it only matchs version with 3 members (x.y.z).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, though the existing tests don't do that, nor do they test the no-match cases. I could add such tests, but at that point I'd want to rewrite this test method to split it up into multiple tests, and I don't think that's in scope for this PR :) I'll add the couple of extra checks, though.

(b'', ('', '')),
):
with open(filename, 'wb') as fp:
Expand All @@ -563,14 +564,29 @@ def test_libc_ver(self):
expected)

# binary containing multiple versions: get the most recent,
# make sure that 1.9 is seen as older than 1.23.4
chunksize = 16384
with open(filename, 'wb') as f:
# test match at chunk boundary
f.write(b'x'*(chunksize - 10))
f.write(b'GLIBC_1.23.4\0GLIBC_1.9\0GLIBC_1.21\0')
self.assertEqual(platform.libc_ver(filename, chunksize=chunksize),
('glibc', '1.23.4'))
# make sure that eg 1.9 is seen as older than 1.23.4, and that
# the arguments don't count even if they are set.
chunksize = 200
for data, expected in (
(b'GLIBC_1.23.4\0GLIBC_1.9\0GLIBC_1.21\0', ('glibc', '1.23.4')),
(b'libc.so.2.4\0libc.so.9\0libc.so.23.1\0', ('libc', '23.1')),
(b'musl-1.4.1\0musl-2.1.1\0musl-2.0.1\0', ('musl', '2.1.1')),
(b'no match here, so defaults are used', ('test', '100.1.0')),
):
picnixz marked this conversation as resolved.
Show resolved Hide resolved
with open(filename, 'wb') as f:
# test match at chunk boundary
f.write(b'x'*(chunksize - 10))
f.write(data)
self.assertEqual(
expected,
platform.libc_ver(
filename,
lib='test',
version='100.1.0',
chunksize=chunksize,
),
)
picnixz marked this conversation as resolved.
Show resolved Hide resolved


def test_android_ver(self):
res = platform.android_ver()
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.