chronotope / chrono

Date and time library for Rust
Other
3.3k stars 523 forks source link

Add: Support wasm32-unknown-emscripten #1541

Closed sevenc-nanashi closed 5 months ago

sevenc-nanashi commented 6 months ago

I added support for wasm32-unknown-emscripten. With this PR, chrono will get timezone support.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.82%. Comparing base (d91b7d4) to head (e466a30). Report is 12 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1541 +/- ## ========================================== + Coverage 91.81% 91.82% +0.01% ========================================== Files 40 40 Lines 18355 18345 -10 ========================================== - Hits 16853 16846 -7 + Misses 1502 1499 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

djc commented 6 months ago

I don't find this PR convincing. The crate you're advocating we start depending on:

I don't think that's a stable base for us to build on.

Apart from that I'd like to hear a rationale for adding an emscripten target, which I think has been superseded in most use cases by wasm32-unknown-unknown and WASI.

ALEX11BR commented 6 months ago

Thank you for working on this!

@ALEX11BR As the author of emscripten-functions would you be willing to comment on if this is the right direction for chrono on emscripten?

Looks mostly OK to me. I had a remark to make with regards to the use of run_script functions where direct bindings to the used JS functions are available.

pitdicker commented 5 months ago

@sevenc-nanashi Sorry for the wait. Could you reply to the concerns of @djc? They match my concerns on discord.

I know little about the current status of emscripten. Stdweb seems to be no more, and emscripten-functions is a crate that doesn't see much use yet.

sevenc-nanashi commented 5 months ago

Oops, sorry for the late reply.

(Disclaimer: I'm not native speaker of english, so I might use some bad language...)

In most cases, using wasm-pack (wasn32-unknown-unknown) or wasi is enough to build a new software. But emscripten has a big advantage: it can compile C source (in other words, crates using cc can work with emscripten) I used emscripten to port the software written in Rust. (It's just my opinion: Isn't it so fascinating that some software written in Rust runs on browser?)

I don't care about closing this PR as lack of use cases.

pitdicker commented 5 months ago

How hard would it be to link to emscripten_run_script_string directly?

pitdicker commented 5 months ago

Hmm. Cargo requires that there is at most one crate that links to a C library. So a project using stdweb is incompatible with emscripten-functions, and either would become incompatible with chrono if we link directly.

sevenc-nanashi commented 5 months ago

emscripten_run_script_string directly?

It shouldn't be so hard I guess.

pitdicker commented 5 months ago

Unfortunately because of the state of the wasm32-unknown-emscripten target I think it is best not to merge this PR for now 😞. Thank you for all your efforts @sevenc-nanashi!