-
Notifications
You must be signed in to change notification settings - Fork 238
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
feat: add Pyodide support #1456
Conversation
@henryiii could you approve the github actions workflows? |
38a706b
to
e6a77ed
Compare
@henryiii is the windows_x86 failure a flake?
|
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.
Thanks a lot for doing this work! It's certainly more work that I anticipated (without knowing much about cibuildwheel) :)
Yes. Some networking issue that is hard to track down. |
BTW, would you like me to rebase for you? I don't mind. |
I already merged main locally. |
Presumably we need documentation somewhere for the fact that Python 311 needs to be installed. |
Splendid work on this so far @hoodmane ! Quality looks really good. Comments above. |
We talked about the Python requirement at PyCon a bit - I think it's best to require 3.11 instead of adding a lot of time & bandwidth for a docker image. Since pyodide is a bit behind official CPython, all CI's should have a suitable host Python available. If PyBI's land, then we might be able to make this simpler. :) I think the name is a bit in the air. Technically, it's emscripten + version + flags. But pyodide selects a specific set, and the hope is it won't update between Python versions. So 3.12 would be a new version of emscripten + flag updates, etc. But I think that's going to be incorrect at least once (I think there's a planned 3.11 update). The hope is by the time wheels are cleared for PyPI, it will be stable. There's also the possibility that CPython will move emscripten to tier 2 (not soon, sadly, looks like only WASI is going to tier 2), in which case CPython could take over the definition of the emscripten version + flags for whatever version it started on. This will need docs - it should be clear this is experimental and you can't upload the wheels to PyPI yet. Setting it up is a bit more involved than just adding it to a matrix, etc. I can help with docs later (traveling at conference this week). Personally and quite selfishly, I really want the cmake bug fixed before it ships, I expect scikit-build/scikit-build-core users will want to make pyodide wheels once this ships in cibuildwheel, and can't do that due to a variable being stripped from CMake's environment by emscripten and pyodide isn't auto-fixing out-of-tree wheels. I can help fix this early next week. |
Thanks for the detailed review @joerick! |
You're talking about @ryanking13 do you see any reason not to call Of course it would be great if we could also fix the issue with Emscripten, but it seems potentially easier to change Pyodide. |
Sounds good. We should try to make out of tree and in tree builds identical. |
There are three fixes, any one of them would be enough:
I'd really like both 1 and 2 if possible. :) |
Okay I think all the integration tests are passing and I think I addressed the most recent round of review comments. So this is waiting on:
@henryiii I'll make a PR to run |
I guess it's failing because of the color / formatting in |
Hmm test passes locally... |
@@ -19,12 +19,11 @@ def test_build_frontend_args(tmp_path, capfd, frontend_name): | |||
project.generate(project_dir) | |||
|
|||
# the build will fail because the frontend is called with '-h' - it prints the help message | |||
add_env = {"CIBW_BUILD_FRONTEND": f"{frontend_name}; args: -h"} | |||
if utils.platform == "pyodide": | |||
add_env["TERM"] = "dumb" # disable color / style |
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.
Hopefully this works? I have no idea why it passes locally though??
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.
Seems that it fixed it indeed =)
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.
it seems too have been style/color as you pointed out.
I don't know why it passes locally either...
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.
Maybe something is checking for the GitHub actions var and assuming it is in a CI run that supports color?
This reverts commit 917646c.
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.
IMO, this is ready to merge, we can do followup patches if we find any improvements. It looks like the two outstanding issues might both be on the pyodide side, so we might not want to make a release until 0.26.1.
Unless anyone has any reservations, will merge tomorrow.
Yes, scikit-hep/boost-histogram#938 (linux) makes it through the "pytest" command and dies on the other bug. But we were seeing this issue in cibuildwheel's tests, even on Linux. |
Thanks everyone! |
Resolves #1454. Also close #1002.
TODO:
Working:
NonPlatformWheelError
asyncio.run