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

basic wasm support #331

Merged
merged 11 commits into from
Aug 23, 2019
Merged

basic wasm support #331

merged 11 commits into from
Aug 23, 2019

Conversation

evq
Copy link
Contributor

@evq evq commented Aug 16, 2019

previous PRs toward wasm support appear stuck ( #287 and #286 - the refactor it depended on ).

I built on the essentials of the work by @jjpe, fixing the local timezone support and adding tests.

jjpe and others added 3 commits August 16, 2019 07:46
While likely providing only incomplete support for WebAssembly, this
commit opens up chrono for use on the wasm32 architecture.
@quodlibetor
Copy link
Contributor

Nice! Thank you, I especially appreciate how tiny this commit is and that it has real tests. The rustwasm community seems like they're doing amazing work to make this so readable.

I haven't been following that community super closely, though, by accepting this will I be accidentally ruling out stdweb, or are they compatible now?

If they're not compatible, could you put the constructors behind a wasm-bindgen feature and make 'wasm-bindgen` optional?

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

I'll verify but I believe it is compatible with stdweb but perhaps not wasm32-unknown-emscripten. (stdweb can use both wasm32-unknown-emscripten and wasm32-unknown-unknown afaik). I can add additional conditionals on target_os if that turns out to be the case.

Finally got CI to pass, I have to admit I was a bit confused so let me know if there's a better way to get it running on the right matrix jobs.

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

Okay after testing with stdweb I've observed the following behavior:

  1. With current master stdweb + wasm32-unknown-emscripten Utc::now() and Local::now() work
  2. With current master stdweb + wasm32-unknown-unknown Utc::now() and Local::now() panic
  3. With this branch stdweb + wasm32-unknown-emscripten Utc::now() and Local::now() panic
  4. With this branch stdweb + wasm32-unknown-unknown Utc::now() and Local::now() panic

We can easily fix 3. by conditioning on target_os as mentioned above.

I believe 4. is actually not actually a stdweb interaction issue - but rather due to my use of cargo web for testing. This apparently can be detected using cargo_web per https://github.com/koute/stdweb/blob/f8035f0114677c07ad9a19da44caadeaf1cfe662/Cargo.toml#L29

I'll confirm by ensuring the stdweb wasm-bindgen-minimal example works. If that's the case we can intentionally fail compilation in the case of wasm32-unknown-unknown + cargo web so at least the user knows at compile time instead of runtime or we can just let it panic (current behavior). I'll go with the former but let me know if you'd prefer the latter.

@quodlibetor
Copy link
Contributor

Thanks! Do you have the code/steps you used to verify that just so that I understand?

I'm mostly confused about how using cargo web could be the problem if doing the same thing currently succeeds in situation 1.

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

Sorry to be clear - situation 1. is broken by this PR presently (as shown in situation 3.) however it is easily fixed by changing all our conditional compilation to check for emscripten in target_os.

Current master:

build tool target_os stdweb result
wasm-pack unknown no panic
wasm-pack unknown yes panic
cargo web unknown yes panic
cargo web emscripten yes works

This branch:

build tool target_os stdweb result
wasm-pack unknown no works
wasm-pack unknown yes works
cargo web unknown yes panic
cargo web emscripten yes works (post fix mentioned above)

As the remaining panic seems to be due to cargo web I am wondering whether we want to keep the runtime panic or detect cargo web + wasm32-unknown-unknown and fail at compile time.

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

As far as my testing methodology, for wasm-pack tests I am running one of the commands added to ci/travis.sh and for stdweb tests I have cloned the repo and am using this patch: https://gist.github.com/evq/f8239238b368653dee634cc9d2f74fae

In examples/todomvc I am running cargo web start --target=wasm32-unknown-unknown and in examples/wasm-bindgen-minimal I am running ./build.sh.

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

koute/cargo-web#92 appears to be the cargo-web issue for wasm-bindgen support

@evq
Copy link
Contributor Author

evq commented Aug 16, 2019

The other option is that it's possible to trigger a compile time warning through a bit of hackery - https://internals.rust-lang.org/t/pre-rfc-add-compile-warning-macro/9370/7

  warning: unused `cargo_web::CompileWarning` that must be used
     --> src/lib.rs:430:33
      |
  430 |         fn trigger_warning () { CompileWarning; }
      |                                 ^^^^^^^^^^^^^^^
  ...
  435 | compile_warning!(cargo_web, "Some chrono functionality currently may not work for wasm32-unknown-unknown + cargo web");
      | ------------------------------------------------------------------------------------------------------------------------ in this macro invocation
      |
      = note: `#[warn(unused_must_use)]` on by default
      = note: Some chrono functionality currently may not work for wasm32-unknown-unknown + cargo web

Perhaps that is better as once the cargo web issue is resolved then chrono will just work albeit with a overzealous warning.

@evq
Copy link
Contributor Author

evq commented Aug 20, 2019

@quodlibetor any thoughts on keeping existing panic at runtime vs warning or error at compile time?

@quodlibetor
Copy link
Contributor

Ah, yeah that seems like a bug in the ecosystem that should just be fixed upstream, similar to if some other library replaced an allocator or something, I don't think that we should add a warning or compile-time error to chrono, despite the fact that I'd rather not include code that I know can break.

So I guess:

  • could you get rid of the compile_error
  • how hard would it be to set up an emscripten test, or to tell me what to do to be able to verify it?

@quodlibetor quodlibetor merged commit 3e180b7 into chronotope:master Aug 23, 2019
@quodlibetor
Copy link
Contributor

Thanks! I got emscripten working for myself and verified for my own sanity that this doesn't regress that.

I'll release this over the weekend! Thanks for pushing this through!

@evq
Copy link
Contributor Author

evq commented Aug 23, 2019

Perfect! I had issues getting cargo web test to play nicely, glad you were able to manually test

@evq
Copy link
Contributor Author

evq commented Aug 23, 2019

@quodlibetor ooof sorry about the commit spam after being merged into master - had I known you were going to merge as is I would have squashed all the commits where I was trying to get travis to pass :)

@quodlibetor
Copy link
Contributor

haha no worries! If I had realized you would be interested in doing that I would have waited :-)

quodlibetor added a commit that referenced this pull request Aug 31, 2019
@mankinskin
Copy link

wasm32-unknown-unknown is still panicking on chrono::offset::Utc::now(). Are there any plans to resolve this? Do we know why this happens?

@quodlibetor
Copy link
Contributor

quodlibetor commented Sep 24, 2019

Are you running with wasm-bindgen? I do not know why it is happening, it seems to work for me.

@mankinskin
Copy link

mankinskin commented Sep 24, 2019

I am using stdweb and yew and cargo-web and building for wasm32-unknown-unknown. stdweb and yew are both depending on wasm-bindgen.

@mankinskin
Copy link

I have now also followed this setup using wasm-pack and wasm-bindgen, and as soon as I add
let now = chrono::offset::Utc::now(); in the greet function, the website runs into a panic:

Error importing `index.js`: RuntimeError: unreachable
    at __rust_start_panic (wasm-function[64]:1)
    at rust_panic (wasm-function[33]:31)
    at std::panicking::rust_panic_with_hook::h38e55c6f9a9b5d9f (wasm-function[14]:303)
    at std::panicking::begin_panic::h10eee402279a32f7 (wasm-function[31]:40)
    at time::sys::inner::get_time::ha8824ffe75172a8b (wasm-function[48]:13)
    at time::get_time::h6490f2cbe72dd17e (wasm-function[42]:14)
    at chrono::offset::utc::Utc::now::h6f8ed24221636f96 (wasm-function[16]:19)
    at greet (wasm-function[34]:14)
    at Module.greet (webpack:///../pkg/budget_app.js?:10:62)
    at eval (webpack:///./index.js?:5:49)

@mankinskin
Copy link

mankinskin commented Oct 6, 2019

The problem is that the time crate does not yet implement get_time() for target_arch="wasm32", as you can see here: https://docs.rs/time/0.1.42/src/time/sys.rs.html#102

I am now using stdweb to get the time from javascript

let timestamp = stdweb::web::Date::now(); // in millis
let secs: i64 = (timestamp/1000.0).floor() as i64;
let nanoes: u32 = (timestamp as u32%1000)*1_000_000;
let naivetime = chrono::NaiveDateTime::from_timestamp(secs, nanoes);
let datetime = chrono::DateTime::<Utc>::from_utc(naivetime, Utc);

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