Skip to content
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

Use webassembly exception handling #5320

Open
wants to merge 74 commits into
base: main
Choose a base branch
from
Open

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 13, 2025

It's a bit hard to make the rust compiler play along, see the upstream PR for information about the rust difficulties pyodide/pyodide-build#81

Based on / supersedes #3817

Build failures

  • python-flint
  • ppl
  • gdal

Import failures

These use setjmp/longjmp and have errors because the symbols for setjmp/longjmp are missing.

  • opencv
  • rasterio
  • optlang
  • fiona
  • swiglpk
  • pyiceberg
  • pygame-ce

@hoodmane hoodmane force-pushed the wasm-eh branch 2 times, most recently from 25955b8 to 3fa4817 Compare January 13, 2025 21:19
@hoodmane
Copy link
Member Author

The only core test failure is test_cpp_exceptions_and_syncify which we'll need to deal with
emscripten-core/emscripten#23395
in order to fix. This isn't exactly a regression either, I think the bug exists on the main branch but this change makes our test hit it.

@hoodmane
Copy link
Member Author

Clarabel has to be disabled until they resolve a problem with the newest beta rust.
oxfordcontrol/Clarabel.rs#154

hoodmane added a commit to hoodmane/pyodide that referenced this pull request Jan 15, 2025
Today's nightly is the first one with all the commits needed for wasm eh pyodide#5320.
Bump the main branch to use the same rust version.
@hoodmane hoodmane requested review from agriyakhetarpal and ryanking13 and removed request for agriyakhetarpal February 4, 2025 09:38
@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2025

@agriyakhetarpal @ryanking13 I think all tests are passing now, would appreciate if you could review this and pyodide/pyodide-build#81.

@ryanking13
Copy link
Member

Thanks for working on this Hood. I'll review it soon, probably before this weekend.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping here, @hoodmane. I can't say I understand all of the details, but it looks good to me – I'll be echoing some of @ryanking13's previous comments.

@agriyakhetarpal
Copy link
Member

I remember that pybind11's tests for Pyodide/WASM downstream were also passing -fexceptions. Should they also switch to -fwasm-exceptions once this PR makes it to a release?

@hoodmane
Copy link
Member Author

Yes everyone downstream needs to pass -fwasm-exceptions or they will get load time errors.

@hoodmane
Copy link
Member Author

But it'll be a bit annoying if they want to build for both Pyodide 0.27 and Pyodide 0.28 since they need different flags.

@agriyakhetarpal
Copy link
Member

Yes, that would be a breaking change for cibuildwheel. Could we document this better in the CHANGELOG? Maybe we can intercept this in pywasmcross, raising a warning and switching it out internally if set? I haven't contributed to that side of our codebase before, so if yes, I would be happy to try that out.

Co-authored-by: Agriya Khetarpal <[email protected]>
@hoodmane
Copy link
Member Author

I should clarify further about sjlj in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants