Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-87414: add musl support to platform.libc_ver#103784
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
cmaureir commented Apr 24, 2023 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
This includes an implementation from the packaging module to detect the musl version on the system, allowing libc_ver to properly return the version rather than an empty tuple. Co-authored-by: Manuel Kaufmann <[email protected]>
humitos commented Apr 24, 2023
We QAed this patch locally and we got it working. However, we are not 100% sure if this is a good implementation and, in the case it is, we are not sure if the place were we copied the functions and put the new file is OK, so:
|
malemburg commented Apr 24, 2023
A few comments:
I have no way to test the code, so can only do source level review. Please find someone who can test this in real life. Thanks. |
cmaureir commented Apr 24, 2023
Thank you Marc 🎉 I addressed the following points:
I naively assumed that due to the code being used inside the packaging module it should have been straightforward, but I totally understand, will be double check :) We will work on this soon with @humitos
Understood, thanks! maybe the person who reported it can give us a hand. |
We get the following error otherwise: ``` ImportError: attempted relative import with no known parent package ```
humitos commented Apr 25, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This is how we are testing this code. I hope it makes sense:
|
malemburg commented Apr 25, 2023
Thanks for the instructions, @humitos. I forgot that Docker containers are an easy way to access Alpine builds :-) BTW: I thought a bit more about adding a new modules to Lib/ and would like to revert my earlier comment. For the small extra logic that is needed, it will create more maintenance burden than is worth it. Could you please try to add the required code directly the platform module ? Perhaps it can be refactored and shortened, since you are not really using all of the parsing logic. Thanks. |
Put the musl tests behind `requires_musl` decorator so they are not run in systems without its support.
humitos commented Apr 25, 2023
I updated the PR to live only in the I wrote some small tests for Please, let us know if there are more changes required 😄 |
malemburg left a comment
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.
Still a few minor things to fix, but overall, this looks good.
Please also add a NEWS entry.
Thanks.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Apr 26, 2023
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
malemburg commented May 7, 2023
The test fails on Macs due to a missing "ldd". On Macs, you typically use "otool" instead. It would be better to simply return False in |
Uh oh!
There was an error while loading. Please reload this page.
mara004 commented Nov 27, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@humitos Thanks for working on this PR. May I ask how this is going on? Seems to be stalled with requested changes since May... |
Uh oh!
There was an error while loading. Please reload this page.
blubberdiblub commented Jan 20, 2024
@cmaureir this pull request seems nearly ready for merging, only 3 small issues (comment for the musl in binary check in Would be really nice if this could be merged soon |
- Making requires_musl compatible with macOS - Add permalinks on the packaging references - Add comment for the b'musl' check before b'libc'
cmaureir commented Jan 28, 2024
Super sorry @malemburg@blubberdiblub I thought this was merged long time ago 😢 |
| @support.requires_subprocess() | ||
| @support.requires_musl() | ||
| def test_libc_ver_musl(self): | ||
| self.assertEqual(platform.libc_ver(), ("musl", "1.2")) |
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.
Will this always be 1.2?
encukou commented Mar 8, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@malemburg, if you want to merge this, please go ahaed. I'll write about what would make me comfortable with merging it. This is quite a lot of code for an unsupported platform.
Getting a platform supported is a bit of catch-22: you need a core dev, and it's hard to become one if you're interested in an unsupported platform. To help solve that, I'd like to figure out how community support for a platform could work. Is anyone here interested in maintaining CPython on musl? Ideally someone interested in eventually becoming a core dev and bringing musl to official Tier 3 support? If so, please add yourself to the experts index with a PR here. The indication that someone wants to help keep CPython working on musl, with Marc-Andre's “this looks good” comment, would convince me to merge this. |
malemburg commented Mar 8, 2024
This is really only a side effect of adding ELF header parsing instead of using the file output. We may be able to make good use of it on other platforms as well. Regarding supporting the musl platform: I think we simply have to accept that CPython will run on unsupported platforms as well 🙂 Alpine is a very widely used platform for running CPython on Docker/Podman, whether we have official tier-X support for it or not. Simply rejecting patches for such a common platform, just because we can't check all the boxes for official tier-X support feels somewhat unreasonable -- at least to me. Of course, happy to learn if someone wants to help check those boxes for Alpine platforms. |
mara004 commented Mar 8, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Given that there has been a successful PEP for musllinux platform tags (656), "unsupported" sounds a bit hyperbolic to me. 😅 Python is definitely being used on musl systems.
Not in CPython itself perhaps, but there are still downstream maintainers -- Alpine packagers would probably be happy to help. |
encukou commented Mar 11, 2024
Sure. Then again, the first thing I'd like to ask a musl maintainer is whether ELF parsing is the proper way to get the libc version.
Yup. We don't test it here, so we can't really take responsibility for fixing it when it breaks. And we don't have musl experts on board. That's an issue to be fixed, and while I can't fix it myself, I can help on the organizational side of things. |
zware commented Apr 28, 2025
As near as I can tell, this has mostly been superseded by GH-131313. Is there anything here that can/should improve on what was done there? |
malemburg commented Apr 28, 2025
Indeed and the solution there is much leaner than this one. I'm going to close it for now. Thanks @cmaureir for investing the time. Please reopen the issue if you think there are still some parts which would be useful to have in the platform module. |
mcepl commented Apr 29, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
For anybody who tries that, let me add that Python officially doesn’t support LibreSSL anymore (e.g., #89532), replace And yes, |
cmaureir commented May 23, 2025
Hey! sorry for being late to the party :P I'm happy this was done anyways, independently if this patch was not the final solution. I know many Alpine Linux (and other musl users) will be happier A big thanks to @humitos for all the time we spend discussing this, testing, and pinging each other in order to have one-more-push to finish it. It was a nice experience. And thanks to everyone that took the time to review what we were doing 🤓 See you all in a future PR! |
This includes an implementation from the packaging module to detect the
muslversion on the system,allowing
libc_verto properly return the version rather than an empty tuple.