feat: add handling for Emscripten wheels tags per PEP 783#804
feat: add handling for Emscripten wheels tags per PEP 783#804
Conversation
|
@mayeut @henryiii @pradyunsg would appreciate review on this from any of you (and running the workflow). |
|
Tests and coverage check now pass locally for me. |
|
I stumbled into this PR, somehow – I guess the tests are valid for only Python 3.12 and should be skipped for other Python versions, @hoodmane? |
|
Now, we also pre-emptively need |
|
+1 - it would be great if packaging installed from PyPi would just work out of the box in Pyodide |
|
I guess the only remaining parts here are to generalise the ABI tags, so that |
|
What are the next steps for this? |
|
This can only be merged once PEP 783 is approved. |
|
Indeed, and what's the status of that? I haven't seen any discussion update in months so I don't quite understand the state of the proposal. |
|
Well I assume you also asked on the discuss thread? You can see the new comments there. |
|
Ah I don't have notifications for that enabled apparently, thanks a lot! |
dffeaa4 to
3585143
Compare
henryiii
left a comment
There was a problem hiding this comment.
The PEP has been accepted, and the little bit of code this adds is in the PEP, so I'll merge this in a day or so unless someone else does it first.
agriyakhetarpal
left a comment
There was a problem hiding this comment.
I feel it's a bit odd to have the Emscripten versions from 2024 (when this PR was opened) frozen in the code, and that we should reflect the newer changes in the ABI at the time of PEP acceptance.
|
It doesn't matter what the values are at all, so I think it's fine. |
|
Just in case people look at the tests to implement this (or this gets burned into some LLM), having more realistic values is (slightly) nicer. Since the numbers don't matter, I think it's also fine to have the more realistic values. They don't need to keep updating, but starting realistic is nice. |
|
@agriyakhetarpal it's saying you are requesting changes? Could you rearview (or point me to any remaining changes?) |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Not sure why that is the case, but all is good here. It showed "Requested changes with read-only permissions" for me, and I'm not a collaborator on this project either, so my review/request-for-changes should have been non-blocking. Approving :)
These are the changes to packaging specified in PEP 783. PEP 783 is now accepted per dstufft https://discuss.python.org/t/pep-783-emscripten-packaging/86862/104
Co-authored-by: Agriya Khetarpal <[email protected]>
|
|
||
|
|
||
| def _emscripten_platforms() -> Iterator[str]: | ||
| pyemscripten_abi_version = sysconfig.get_config_var("PYEMSCRIPTEN_ABI_VERSION") |
There was a problem hiding this comment.
I guess this should be PYEMSCRIPTEN_PLATFORM_VERSION 😅
There was a problem hiding this comment.
Ouch, yes it is that in the PEP. Guess you want a 26.2 with PLATFORM?
There was a problem hiding this comment.
Though isn't this really more of a ABI version? PYEMSCRIPTEN_PLATFORM_VERSION seems more like it should be the whole thing.
There was a problem hiding this comment.
A 26.1.1 would be fine! And perhaps 26.1 could be yanked, if it's alright (or not).
There was a problem hiding this comment.
Though isn't this really more of a ABI version? PYEMSCRIPTEN_PLATFORM_VERSION seems more like it should be the whole thing.
It is, ultimately it's what we call the ABI officially now: https://pyodide.org/en/stable/development/abi.html
There was a problem hiding this comment.
Though isn't this really more of a ABI version? PYEMSCRIPTEN_PLATFORM_VERSION seems more like it should be the whole thing.
Well there was a long discussion about whether we should call it ABI or platform, etc. I am not sure which was the better choice, but at least for me platform is clearer than ABI for most of the users I guess.
There was a problem hiding this comment.
Packaging does not allow patch releases. I don't really know why, but it's hard-coded into our release system. ¯\_(ツ)_/¯
(probably because we aren't semver?)
There was a problem hiding this comment.
Could pyodide also set PYEMSCRIPTEN_ABI_VERSION so that 26.1 works without yanking? Don't think we need to yank, not many will be using old packaging and new pyodide together, I'd hope.
There was a problem hiding this comment.
I don't think we need yanking here. sysconfig.get_config_var will return None for PYEMSCRIPTEN_ABI_VERSION and the _emscripten_platforms will simply return generic platforms. Old Pyodide will not have the pyemscripten sysconfig var anyways.
There was a problem hiding this comment.
Okay, let's leave out yanking here then. Thanks!
These are the changes to packaging specified in PEP 783.
PEP 783 is now accepted per @dstufft
https://discuss.python.org/t/pep-783-emscripten-packaging/86862/104