gh-138130: Fix return value of libc_ver() on Emscripten #138132
Conversation
|
!buildbot emscripten |
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 29fb6c0 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138132%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this comment.
Implementation looks good to me. It's also exciting to see the buildbot back in the green - although it highlights a new problem with the PyRepl test...
The only missing part is testing. There's an existing test in Lib/test/test_platform.py that validates the output of libc_ver, which is skipped on emscripten. This seems like as good an opportunity as any to add an extra test (or modify the existing one) that (a) verifies the new implementation works, and (b) formally validates that the build is returning the right Emscripten version. Since our 3.14 builds must always return Emscripten 4.0.12 (and for now, so do 3.15 builds), we can add a formal check for that.
On top of straightforward coverage, this has the added advantage that it protects us from inadvertently bumping a server configuration and rendering an Emscripten build unusable because of an ABI change.
|
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 |
|
Actually, I'm not sure if this is a good idea to return the emscripten version in libc_ver For me, the doc is clean enough If we return the emscripten version in this function. I think we will make it more confusing. |
|
Emscripten includes a funky libc with it, so I think it makes sense. |
Agreed. The alternative would be to return For me, it's analogous to the situation with Android. Yes, technically, Android is "a Linux" - but blurring Android in to a Linux descriptor hides a significant platform distinction in the hope that code that is "pure" Linux will "just work". It's better to provide a clear distinction, and require a "if Linux or Android" check as an opt-in. |
|
Before I would have said call it musl but now Emscripten is using a hybrid of musl and llvm libc and is trying to transition all the way to an unpatched llvm libc in the long term. Once they are done with this process we can say it's llvm libc. For now, I think Emscripten libc is the best answer. |
|
!buildbot emscripten |
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 2bd3ac5 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138132%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this comment.
Test looks good; the only issue at this point is the failure on the buildbot, which appears to be caused by python/buildmaster-config#616. I've got a possible fix in flight; given this is the last fix for a clean build, I'd like to see it run clean :-)
|
!buildbot emscripten |
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 2bd3ac5 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138132%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
Hrm - that tweak to the buildbot didn't work... I'm open to suggestions on why it could be failing... |
There was a problem hiding this comment.
Thanks to some help from @encukou, the buildbot is working again!
|
Thanks @hoodmane for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…GH-138132) Emscripten's libc is a hybrid of musl and llvm libc; but it reports that it is "glibc". This modifies the return value of `platform.libc_ver()` to return something that is Emscripten-specific. (cherry picked from commit 11a5fc8) Co-authored-by: Hood Chatham <[email protected]>
|
GH-138312 is a backport of this pull request to the 3.14 branch. |
Uh oh!
There was an error while loading. Please reload this page.