-
Notifications
You must be signed in to change notification settings - Fork 253
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
PEP 656 musllinux support #411
Conversation
This comment has been minimized.
This comment has been minimized.
Alright, I figured out how to do build test binaries from Docker and wrote tests against them. Logic-wise I think this is ready, but there’s some duplicated logic parsing ELF header between |
cfeeff0
to
30705f7
Compare
Is there a benefit to testing the tags returned when running in a |
Maybe, but probably marginal. I didn’t do it (or |
lines = [n for n in (n.strip() for n in output.splitlines()) if n] | ||
if len(lines) < 2 or lines[0][:4] != "musl": | ||
return None | ||
m = re.match(r"Version (\d+)\.(\d+)", lines[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth compiling the regex globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiled versions of the most recent patterns passed to re.match(), re.search() or re.compile() are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions.
Meh, it can't hurt but I won't block the PR on that (which, I guess is the same as you).
Ready if the tests pass! |
@pradyunsg did you want to review at all? Otherwise I'm happy to accept this. |
No concerns on merging this without me reviewing. |
I just realized it's probably better to wait until https://www.python.org/dev/peps/pep-0656/ is actually accepted. 😄 |
Hey @brettcannon, friendly reminder - PEP-656 is in, is there anything else needed to merge this? |
@MrMino CI to pass again with the update to match |
Sounds like someone just needs to push the button...? 🙃 |
Unfortunately there looks to have been a bad merge from |
Actually, not a bad merge; this PR predates the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a reference to distutils in tests/test_tags.py
.
Tests coming soon...
This is probably slightly faster and more robust than the ldd method since it does not require ldd on PATH, and avoids some terminal encoding issues. The ldd approach is kept in edge cases where the executable is somehow not readable. I suspect ldd would not be very useful in this scenario either, but there's not harm being safe?
Thanks! |
https://discuss.python.org/t/pep-656-platform-tag-for-linux-distributions-using-musl/7165/32