Add CI tests for wasm32-emscripten wheels in Pyodide #244
Conversation
|
I'm keeping this as a draft because IMO it needs some improvements before it can merge but I would still appreciate feedback if you get around to it. @messense @davidhewitt |
There was a problem hiding this comment.
Thanks for working on this and sorry for the long delay in review. Been busy either side of falling ill.
Looks solid to me, really cool to see this working. There appears to be a rust_with_cffi failure when trying to build cffi, maybe it makes sense to skip that one for now?
| CARGO_TARGET_WASM32_UNKNOWN_EMSCRIPTEN_LINKER=str( | ||
| emscripten_dir / "emcc_wrapper.py" | ||
| ), | ||
| PYO3_CONFIG_FILE=str(emscripten_dir / "pyo3_config.ini"), |
There was a problem hiding this comment.
From this should I infer that pyo3_build_config doesn't configure for pyodide correctly? Do you think we would be able to upstream a fix?
There was a problem hiding this comment.
From this should I infer that pyo3_build_config doesn't configure for pyodide correctly?
I am not sure. I think it may just be that I don't know how to use the tool correctly. How would I check this?
There was a problem hiding this comment.
So usually in cross-compile situations like for emscripten the pyo3_build_config build scripts will try to look for a sysconfigdata file and configure from that. Alternatively, can set the abi3-py310 feature and then it will do a "standard" cross compile, which might just work.
I view the PYO3_CONFIG_FILE env var as an escape hatch when nothing built-in works, so if either of the above work (or can be made to work) that's probably preferable.
noxfile.py
Outdated
| emscripten_dir / "emcc_wrapper.py" | ||
| ), | ||
| PYO3_CONFIG_FILE=str(emscripten_dir / "pyo3_config.ini"), | ||
| RUSTFLAGS=" ".join( |
There was a problem hiding this comment.
It might be worth having a .cargo/config.toml with a [target.wasm32-unknown-emscripten] section for these? Or alternatively setuptools-rust could set them automatically internally?
There was a problem hiding this comment.
Well we are working on upstreaming this stuff:
https://github.com/rust-lang/rust/pulls?q=author%3Ahoodmane
I don't know what the best thing to do before the upstream fixes go in. Unfortunately it looks like the merge window for Rust 1.63.0 is closing today. The fixes will probably go into into 1.64.0.
There was a problem hiding this comment.
Ah, very nice. I think it's ok for us to keep this as-is for now then; if things will eventually "just work" on a new enough rustc I think that's good enough for now.
|
This can be simplified quite a lot now similar to this PR on maturin: |
.github/workflows/ci.yml
Outdated
| @@ -3,6 +3,7 @@ on: | |||
| push: | |||
| branches: | |||
| - main | |||
| - emscripten-ci2 | |||
There was a problem hiding this comment.
Reminder to remove this when ready to merge.
noxfile.py
Outdated
| _PYTHON_SYSCONFIGDATA_NAME="_sysconfigdata__emscripten_wasm32-emscripten", | ||
| _PYTHON_HOST_PLATFORM="emscripten_3_1_14_wasm32", | ||
| CARGO_BUILD_TARGET="wasm32-unknown-emscripten", | ||
| CARGO_UNSTABLE_BUILD_STD="true", |
There was a problem hiding this comment.
Looking at the CI failure and docs for build-std I think this actually might want to be CARGO_UNSTABLE_BUILD_STD="core,alloc,std,proc_macro,test". Or presumably we can drop this soon-ish when your upstream Rust PRs merge.
davidhewitt
force-pushed
the
emscripten-ci2
branch
from
656ff67 to
afe6beb
Compare
davidhewitt
force-pushed
the
emscripten-ci2
branch
from
afe6beb to
97fe8e9
Compare
|
Rebased this and removed some of the tweaks which I think are now covered by upstream rustc; let's see if we can get this merged. |
|
Hmm, emscripten failing with missing symbols. @hoodmane do you know if there's a flag for allowing the undefined symbols through (presumably they are provided by Pyodide at runtime), similar to |
|
I suspect it didn't see |
|
Ah in which case I removed too many RUSTFLAGS - thanks 👍 |
|
Yeah I think |
davidhewitt
force-pushed
the
emscripten-ci2
branch
from
41823b3 to
09ce722
Compare
davidhewitt
force-pushed
the
emscripten-ci2
branch
2 times, most recently
from
cbcfe82 to
83eb2d8
Compare
davidhewitt
force-pushed
the
emscripten-ci2
branch
from
83eb2d8 to
a0e5c20
Compare
|
Finally working! |
|
Thanks @messense and @davidhewitt! |
|
Cheers, appreciate you kicking this off for us, I've got a much better idea how we do (pyodide) support now going forward 👍 |
This is another attempt to add emscripten tests. We generate wheels "by hand" and then test them in Pyodide. This approach could benefit from improvements to Pyodide, it's currently a bit ad-hoc. Would still appreciate review.
The tests work for
html-py-ever,namespace_package, andrust_with_cffi, but I couldn't gethello-worldto build so I am not testing it.