-
Notifications
You must be signed in to change notification settings - Fork 11
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
Build with wasm exception handling #81
base: main
Are you sure you want to change the base?
Conversation
For now, we need to use a custom rust toolchain for this. Apparently all alternatives to this don't work. -Zbuild-std doesn't work with panic=abort (rust-lang/cargo#7359) and my attempts to use a custom sysroot with either https://github.com/RalfJung/rustc-build-sysroot/ or https://github.com/DianaNites/cargo-sysroot/ seem to hit the same problem as with `-Zbuild-std`. Thus, I think the only reasonable way to go is to build the sysroot from the rust source directory. Perhaps we can eventually approach this by copying the `lib/rustlib/wasm32-unknown-emscripten/lib/` folder out of the build of the rust compiler on top of a nightly install of the compiler. For now, there is the additional problem that we need this patch to fix unwind=abort: rust-lang/rust#135450 I got my copy of the rust compiler by checking out this commit: hoodmane/rust@052ba16 two commits ahead of the rust main branch and running: ``` ./x build --stage 2 --target x86_64-unknown-linux-gnu,wasm32-unknown-emscripten ```
for more information, see https://pre-commit.ci
It's a bit hard to make the rust compiler play along, see the upstream PR for the rust difficulties pyodide/pyodide-build#81
It's a bit hard to make the rust compiler play along, see the upstream PR for the rust difficulties pyodide/pyodide-build#81
Thanks for dealing with rust issues. I think we need some more discussion on how to support custom Rust toolchains. Instead of hard-coding the values in the code, it would be nice to make this configurable, using Also, adding a new subcommand that can install some build tools, including rust toolchain can be useful IMO. There was an open issue about it (pyodide/pyodide#3287). |
This also only works for x86-linux. I think what I have to do is build the rust compiler from source, then upload the |
But for that to work, we have to wait first for a nightly with rust-lang/rust#135450. |
Another question is, when can we remove the custom rust toolchain and use the upstream one? I am okay with maintaining the rust toolchain and custom sysroot for a while, but I don't think we can do that forever. So it would be nice to keep track of the blockers. Could you please open an issue that lists the blockers and their progress that needs to be resolved for the wasm exception handling? |
The plan is that we roll out wasm exception handling, check that there are no show stoppers, and then rust switches to using it by default after giving other people a chance to comment if it would cause them trouble. |
Once Rust turns it on by default, we can in principle use the normal Rust toolchain, with the one concern being that it could have abi mismatches due to being compiled with a different Emscripten version than we link with. See the relevant MCP here: |
I'll open tracking issues for the left over problems when we merge this. |
Also, @ryanking13 presumably we need to support both the |
Though I don't know if we want to make the Emscripten updates at the same time or just have a |
Currently the only extra commit I need on top of the Rust main branch to make the appropriate toolchain is this: |
It's a bit hard to make the rust compiler play along, see the upstream PR for the rust difficulties pyodide/pyodide-build#81
Maybe let's do both updates and call it |
Rather than installing the rust toolchain in each build step that uses rust, do it once in buildall. Split off from #81.
87813c5
to
607bd9e
Compare
607bd9e
to
0c88c0f
Compare
for more information, see https://pre-commit.ci
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! Codewise looks okay to me. I left some minor comments.
pyodide_build/config.py
Outdated
@@ -228,7 +230,8 @@ def _get_make_environment_vars(self) -> Mapping[str, str]: | |||
"rustflags": "-C link-arg=-sSIDE_MODULE=2 -C link-arg=-sWASM_BIGINT -Z link-native-libraries=no", | |||
"cargo_build_target": "wasm32-unknown-emscripten", | |||
"cargo_target_wasm32_unknown_emscripten_linker": "emcc", | |||
"rust_toolchain": "nightly-2024-01-29", | |||
"rust_toolchain": "nightly-2025-01-15", |
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.
Let's mention the default rust toolchain update in the changelog.
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.
This change isn't really needed to make this work. Maybe I can revert it here and put it by itself in a separate pr as a follow-up.
@@ -77,6 +77,7 @@ class CrossCompileArgs(NamedTuple): | |||
target_install_dir: str = "" # The path to the target Python installation | |||
pythoninclude: str = "" # path to the cross-compiled Python include directory | |||
exports: Literal["whole_archive", "requested", "pyinit"] | list[str] = "pyinit" | |||
abi: str = "" |
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.
Let's add a comment about this parameter.
if abi > "2025" and arg in ["-lfreetype", "-lpng"]: | ||
arg += "-wasm-sjlj" |
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.
Let's add a comment about the context of this if condition.
) | ||
toolchain_root = Path(result.stdout).parents[1] | ||
rustlib = toolchain_root / "lib/rustlib" | ||
install_token = rustlib / "wasm32-unknown-emscripten_install-url.txt" |
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.
Is this file something you added manually in your custom toolchain? If so, let's add a comment about it.
if arg.startswith("-l"): | ||
arg = replay_genargs_handle_dashl(arg, used_libs, build_args.abi) |
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.
Not a strong opinion, but as build_args.ldflags
is passed by the user, I think the user is responsible for setting the proper flags.
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.
I have mixed feelings. I guess we can't fix static/dynamic libs not built with the python build system anyways so it's not that valuable. But it's pretty weird and annoying that linking -lpng
successfully links but will break with opaque errors at runtime and the fix is to link with -lpng-wasm-sjlj
. Maybe we can add some logic to audit wheel or somewhere appropriate that detects files linked against wrong libraries and explains as best we can what went wrong. Or people can just ask.
So yeah probably it makes sense to remove this.
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 we can add some logic to audit wheel or somewhere appropriate that detects files linked against wrong libraries and explains as best we can what went wrong.
Yeah, I think the problem is that we are not building libraries like libpng
or libfreetype
ourselves and use flags like -s USE_LIBPNG=1
instead. So, there exist multiple variants of those libraries, and it isn't easy to know what to use without looking into the emscripten code.
Ideally, I think it is better to drop all emscripten ports and build libraries ourselves. I think this would fix the issue with the Emscripten version and library version being fixed, and it would also fix the variant issues like the wasm exception.
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.
BTW, I noticed that the library suffices are changed in the recent version of Emscripten: https://github.com/emscripten-core/emscripten/blob/242af002f8dee0366e86002fec187d447dcd26e5/tools/ports/libpng.py#L12-L16
For now, we need to use a custom rust toolchain for this. Apparently all alternatives to this don't work. -Zbuild-std doesn't work with panic=abort (rust-lang/cargo#7359)
and my attempts to use a custom sysroot with either https://github.com/RalfJung/rustc-build-sysroot/ or https://github.com/DianaNites/cargo-sysroot/ seem to hit the same problem as with
-Zbuild-std
. Thus, I think the only reasonable way to go is to build the sysroot from the rust source directory. Perhaps we can eventually approach this by copying thelib/rustlib/wasm32-unknown-emscripten/lib/
folder out of the build of the rust compiler on top of a nightly install of the compiler. For now, there is the additional problem that we need this patch to fix unwind=abort:rust-lang/rust#135450
I got my copy of the rust compiler by checking out this commit: hoodmane/rust@052ba16 two commits ahead of the rust main branch and running: