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

Set up CI for wasm32-emscripten target #2436

Merged
merged 4 commits into from Jun 8, 2022
Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Jun 5, 2022

This adds CI tests for wasm32-emscripten target. CI run builds libpython3.11.a for the wasm32-emscripten target and then runs cargo test against it.
Resolves #2412.

  • fix or xfail broken tests

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 5, 2022

So the segfault seems to be because PyCapsule_Import("datetime.datetime_CAPI") returns NULL.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 5, 2022

It would be good to return an error from PyDateTime_IMPORT if the import fails.

@hoodmane hoodmane force-pushed the emscripten branch 2 times, most recently from be42346 to 7ec96a5 Compare June 6, 2022 05:53
.github/workflows/ci.yml Outdated Show resolved Hide resolved
This adds CI to build libpython3.11 for wasm32-emscripten and
running tests against it. We need to patch instant to work
around the emscripten_get_now:
sebcrozet/instant#47

We also have to patch emscripten to work aroung the "undefined
symbol gxx_personality_v0" error:
emscripten-core/emscripten#17128

I set up a nox file to download and install emscripten,
download and build cpython, set appropriate environment variables
then run cargo test. The workflow just installs python, rust,
node, and nox and runs the nox session.

I xfailed all the test failures. There are problems with datetime.
iter_dict_nosegv and test_filenotfounderror should probably be
fixable. The tests that involve threads or asyncio probably can't
be fixed.
@davidhewitt
Copy link
Member

davidhewitt commented Jun 7, 2022

Thanks, will try to give full review later!

It would be good to return an error from PyDateTime_IMPORT if the import fails.

We shouldn't change the ffi symbol, however in ensure_datetime_api in types/datetime.rs we should definitely assert the API is non-null after import.

Maybe worth correcting as a separate PR and documenting as a fix in the CHANGELOG.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 7, 2022

Maybe worth correcting as a separate PR and documenting as a fix in the CHANGELOG.

Okay I will open a pr. I guess I should add a changelog entry here too.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This is really neat; I'm very excited to see this added to our CI. Thank you so much for putting the time in for this! (I really need to get my head around how shared libraries work in wasm sometime.)

I guess I should add a changelog entry here too.

As strictly speaking there's not anything user-facing here I don't think a CHANGELOG is necessary for this one. Any subsequent MRs where we fix issues with wasm which users will encounter would be worth documenting. (e.g. the datetime fix.)

I've added a few comments - I think we can drop the instant patch, and also I'd like to have ignore = "..." notes on tests which we know will never work.

Let's keep #2412 open after this merges, and then the remaining tests which we believe should work but currently don't can be tracked there. Hopefully we can close everything off before the 0.17 release. (I've still got a couple of axes of feature work I'd like to finish first, so there's some time!)

tests/test_class_basics.rs Outdated Show resolved Hide resolved
tests/test_class_basics.rs Show resolved Hide resolved
tests/test_class_basics.rs Show resolved Hide resolved
tests/test_compile_error.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@mejrs
Copy link
Member

mejrs commented Jun 7, 2022

I'd like to have ignore = "..." notes on tests which we know will never work.

I've always used #[ignore] to disable tests that are very expensive to run, rather than to not run tests that don't work at all. After all, what is the point of cargo test -- --ignored then? So I would personally prefer:

#[cfg(not(target_arch = "wasm32"))]
#[test]
fn test() {
    // ...
}

@davidhewitt
Copy link
Member

I've always used #[ignore] to disable tests that are very expensive to run, rather than to not run tests that don't work at all. After all, what is the point of cargo test -- --ignored then? So I would personally prefer:

Good point, yes, tests which don't ever expect to work should use #[cfg(not(...))] - my bad 😄

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This is really great, thanks again! 💯

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 8, 2022

Thanks for the help and the quick reviews @messense and @davidhewitt! Hopefully I'll get to some followup PRs soon.

@hoodmane hoodmane deleted the emscripten branch June 8, 2022 05:42
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.

wasm32-emscripten support
4 participants