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

Upstream issues for wasm32-emscripten support: instant library and emscripten_get_now #2417

Closed
hoodmane opened this issue Jun 1, 2022 · 6 comments

Comments

@hoodmane
Copy link
Contributor

hoodmane commented Jun 1, 2022

There is a dependency chain pyo3 ==> parking-lot ==> instant. On wasm32-emscripten, instant tries to use _emscripten_get_now which is misspelled: it should be emscripten_get_now.

Issue: sebcrozet/instant#35
PR: sebcrozet/instant#47

It is possible to work around this via JavaScript: on the Emscripten module one can set Module.__emscripten_get_now = Module._emscripten_get_now and it will work.

There is a second bug related to emscripten_get_now in Emscripten: emscipten_get_now has no signature so it won't work if you dynamically link the PyO3 module. This is now fixed upstream.
emscripten-core/emscripten#17123
There is also a JavaScript workaround for this:
Module._emscripten_get_now.sig = "d"

@davidhewitt
Copy link
Member

Thanks for sharing. It won't be pretty, however if necessary we could consider dropping parking_lot dependency on wasm and using the mutexes from std instead. We make use of const_mutex, which std doesn't have, a possible replacement might be something like once_cell::sync::Lazy<std::sync::Mutex<_>> to enable lazy-initialisation with some runtime overhead.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 3, 2022

This problem only occurs with dynamic linking, so if we use static linking for the CI job then we won't need to fix this.

Maybe we could just use a dependency override for instant with the two underscores removed until instant is updated.

@davidhewitt
Copy link
Member

davidhewitt commented Jun 4, 2022

Static linking has generally proved tough due to the need to align Rust / C compilers, it might be easier on wasm but I don't know.

A dependency override can work.

I guess the downside of either approach is that I don't think it'll help in the general case for downstream projects? Finding an alternative implementation seems like the only robust solution.

@davidhewitt
Copy link
Member

As per #2436 (comment), this isn't a problem when using PyO3 0.16 and newer with parking_lot 0.12. Will close this.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 8, 2022

Too bad cryptography is stuck on PyO3 0.15, but it's great that it's fixed in newer versions. Thanks.

@mejrs
Copy link
Member

mejrs commented Jun 19, 2022

We make use of const_mutex, which std doesn't have, a possible replacement might be something like once_cell::sync::Lazy<std::sync::Mutex<_>> to enable lazy-initialisation with some runtime overhead.

It is const now: rust-lang/rust#97791, which looks like it'll be stable in 1.63

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

No branches or pull requests

3 participants