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

Add initial support for the wasm32 arch #287

Closed
wants to merge 6 commits into from
Closed

Conversation

jjpe
Copy link
Contributor

@jjpe jjpe commented Nov 4, 2018

Building on PR #286, this PR adds initial support to chrono for the wasm32 architecture.
Specifically, it adds wasm32-compatible versions of the Local::now() and Utc::now() constructors.

EDIT: merging this would allow this issue to be closed as well.

I've discussed this with one of the maintainers (Alex Chrichton), and
with his blessing I've purged the dependency on the deprecated time
crate in favor of folding its code into chrono.

This enables further refactoring.
The little buggers snuck through.
While likely providing only incomplete support for WebAssembly, this
commit opens up chrono for use on the wasm32 architecture.
@oleid
Copy link

oleid commented Nov 8, 2018

Is there a way to return a datetime<Utc> to JS? I'm running into the following error:

121 | #[wasm_bindgen]
    | ^^^^^^^^^^^^^^^ the trait `wasm_bindgen::convert::IntoWasmAbi` is not implemented for `std::boxed::Box<[(chrono::DateTime<chrono::Utc>, usize)]>`

when trying to return a Vec<(datetime<Utc>, usize)>.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 8, 2018

@oleid That functionality is not covered by this PR.
The best way I can think of for you to deal with more complex types ATM is to put a #[wasm_bindgen] attribute on a new custom type somewhere in your own crate that wraps (chrono::DateTime<chrono::Utc>, usize) or something like it.

@jjpe jjpe mentioned this pull request Nov 25, 2018
@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

Does this PR still support both stable rust and the non-clock / non-wasm-bindgen usecase? Cause that would very much mean that I can't use chrono anymore then.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 25, 2018

With this PR I can use chrono as I would have before, except compiling for WASM does not fail anymore, and the Local::now() and Utc::now() constructors actually work, by delegating to javaScript's Date type.
Note that this builds on a PR that absorbs the time crate functionality into chrono.

Does that answer your question?

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

It does not. Can I use this with wasm32-unknown-unknown on stable Rust? And does this force using wasm-bindgen on me, which is not something I want, as I'm using it in the true "unknown" / "no-std" way. Not in the implicit "wasm32-unknown-web" way.

If this works fine on stable rust and the wasm-bindgen dependency doesn't interfere with the "unknown" use case, then I'm fine with this change as is.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 25, 2018

Can I use this with wasm32-unknown-unknown on stable Rust?

Yes, this is the main point of this PR.
The Cargo.toml dependency entry checks for the wasm32 arch, nothing more.
So wasm32-unknown-unknown or wasm32-unknown-[whatever] shouldn't matter much.

And does this force using wasm-bindgen on me

Yes, because as far as I can see it's pretty much the only way to interact with WASM without repeating the work done by wasm-bindgen, which would be wasteful in code and in time.

I'm using it in the true "unknown" / "no-std" way. Not in the implicit "wasm32-unknown-web" way.

I'm not sure what you mean by this.

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

The wasm32-unknown-unknown target should not imply compiling to the web. wasm has more use cases than that. Unfortunately at the moment Rust doesn't have a -web target, and lots of documentation seems to indicate that wasm-bindgen is how you use the -unknown target, but that's just one way of using it. This is similar to including std in a no-std target. The -unknown target explicitly does not target the web, so just like putting std in no-std breaks everyone's use case that isn't targeting std or in this case the web.

However chrono already has a solution for this, the clock feature. With the feature not enabled, chrono only compiles in the no-std like code it can support, which means not exposing anything that would query the underlying environment. So to still properly support the wasm32-unknown-unknown target, your PR should only depend on wasm-bindgen when the clock feature is enabled, just like with any of the other targets.

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

The Cargo.toml dependency entry checks for the wasm32 arch, nothing more.
So wasm32-unknown-unknown or wasm32-unknown-[whatever] shouldn't matter much.

Then that also breaks wasm32-unknown-emscripten, which does not support wasm-bindgen afaik.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 25, 2018

The wasm32-unknown-unknown target should not imply compiling to the web

This PR has been verified to work on both NodeJS and in any WASM-enabled browser (I tested Chrome and Firefox), the 2 places I needed to test. It is not feasible to test random personal projects of the "let's build a WASM interpreter" variety.
Insofar NodeJS does not count as web this PR should be compatible with your personal needs.
However, the only way currently to get time information on wasm is via JavaScript's Date API, so if that does not work for you you'll have to add that infrastructure to your non-web wasm interpreter as well, which seems like a very nontrivial amount of work to the point you might as well replace chrono with a custom lib / fork.

Then that also breaks wasm32-unknown-emscripten, which does not support wasm-bindgen afaik.

That is not possible, as something that does not even exist yet (i.e. wasm32 support in any form) by definition cannot be broken.

So to still properly support the wasm32-unknown-unknown target, your PR should only depend on wasm-bindgen when the clock feature is enabled, just like with any of the other targets.

This is not currently implemented (at least consciously), but may well be doable with little effort.
Note that this would mean that when clock is not enabled, wasm support would go out the window due to a lack of a date/time source, so this PR would not help you anyway.

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

That is not possible, as something that's not even supported yet (i.e. wasm32 in any form) by definition cannot be broken.

wasm32-unknown-emscripten was supported, and this PR breaks it.

It is not feasible to test random personal projects of the "let's build a WASM interpreter" variety.

That's not what wasm32-unknown-unknown not targeting the web implies. What it implies is that there's no environment you can reliably import from, because for example the actual binary part of the project decides which imports and exports exist and how they are named. This is a more than valid way to use the wasm32-unknown-unknown target that does not necessarily imply a "non-web wasm interpreter". This runs just fine in a browser or node.js. It just doesn't use wasm-bindgen which is just one of many flavors of generating the bindings. Another being stdweb, which probably gets broken by this PR too. Considering wasm-bindgen is just one of many ways of going about defining the imports / exports at this point in time, it should absolutely be gated by a feature flag that shouldn't be forced upon users. Chrono works just fine with stdweb and binary defined imports / exports right now, and you are breaking both of these use cases. It certainly makes sense to have better support for wasm-bindgen, but breaking every other use case is not the way to go at this point in time.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 25, 2018

wasm32-unknown-emscripten was supported, and this PR breaks it.

Source? I've never seen or heard of this. Also, isn't the wasm32-unknown-emscripten target in the process of being phased out in favor of wasm?

That's not what wasm32-unknown-unknown not targeting the web implies. What it implies is that there's no environment you can reliably import, because for example the actual binary part of the project decides which imports and exports exist and how they are named. This is a more than valid way to use the wasm32-unknown-unknown target that does not necessarily imply a "non-web wasm interpreter". This runs just fine in a browser or node.js. It just doesn't use wasm-bindgen which is just one of many flavors of generate the bindings. Another being stdweb, which probably gets broken by this PR too. Considering wasm-bindgen is just one of many ways of going about defining the imports / exports at this point in time, it should absolutely be gated by a feature flag that shouldn't be forced upon users. Chrono works just fine with stdweb and binary defined imports / exports right now, and you are breaking both of these use cases. It certainly makes sense to have better support for wasm-bindgen, but breaking every other use case is not the way to go at this point in time.

Without any environment to import, there's no date/time source, especially on non-std envs.
I don't see how chrono has any chance of working without such a source, as dates, times and by extension durations cannot even be created. So no, it would not run on the web.

As for the millions of other possible configs, that's way outside the scope of this PR, which is essentially only concerned with making chrono, stable std-enabled rust, and wasm play nicely together using the path of least resistance. Hence "initial" wasm32 support.

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

Source? I've never seen or heard of this. Also, isn't the wasm32-unknown-emscripten target in the process of being phased out in favor of wasm?

I'm not sure I can link any source. But I actively use it and it works fine. Emscripten just provides a libc that has all the functions necessary and the time crate and std just use libc.

Without any environment to import, there's no date/time source, especially on non-std envs.
I don't see how chrono has any chance of working without such a source. So no, it would not run on the web.

That's what the clock feature is for. Chrono explicits wants to support use cases where there is no specific environment to get the time from. That doesn't mean you can't use chrono. You can still create time stamps from times sourced through other means in such a no-std like environment. Which in the wasm32-unknown-unknown use case would for example be through an explicit "get_time" like import created by the binary. The clock feature is an explicit goal of the chrono library. You can't just randomly break it with this PR.

@jjpe
Copy link
Contributor Author

jjpe commented Nov 25, 2018

I'm not sure I can link any source. But I actively use it and it works fine.

There's a difference between something being supported, and something working essentially by accident i.e. without the maintainer(s) intending to do so. For example, the former means that it should keep working lest it become a broken feature, but the latter does not. Compare the 2nd case to an internal API: you can use it and it'll work, but the risk of breakage is yours to take.
If a feature is supported, typically the devs will have announced it somewhere.

As for the clock feature, there I tend to agree with you.
But I cannot test that as I don't have any no-std envs available, so I can't properly write the code either. Care to commit the changes?

@CryZe
Copy link
Contributor

CryZe commented Nov 25, 2018

Okay, so I tested your branch against a few use cases and it seems wasm32-unknown-emscripten still compiles, but I didn't verify yet if wasm-bindgen has any negative effects that would break emscripten's bindings. It shouldn't be hard to make that target work again though, no matter whether intentionally supported by chrono or not.

wasm32-unknown-unknown still compiles on stable without any trickery just fine too, except it adds a whole lot of symbols including one import that shouldn't be there, so I guess if you aren't willing to feature flag wasm-bindgen (which could even be on by default), then I'll probably contribute that.

@Pauan
Copy link

Pauan commented Nov 25, 2018

@jjpe The wasm32-unknown-unknown target is used for many things other than wasm-bindgen: e.g. there is also stdweb, which is quite popular (and has many features that wasm-bindgen doesn't have).

So as @CryZe said, this needs to be behind a feature flag, to allow for alternate uses of the wasm32-unknown-unknown target.

@oleid
Copy link

oleid commented Nov 26, 2018

FWIW: It would seem, that stdweb is going to be based uppon wasm-bindgen (or friends) eventually : rustwasm/team#226

@jjpe
Copy link
Contributor Author

jjpe commented Nov 26, 2018

I'll probably contribute that.

@CryZe I encourage you to do so, as I'm currently unable to spend time coding and testing this.

@quodlibetor
Copy link
Contributor

@CryZe @jjpe what are you currently using to verify that chrono works for you? Despite the fact that I'm very eager to merge this, I'm definitely not going to without any tests. I would like to verify that every wasm32-unknown-* (which is just -unknown and -emscripten right now, AFAIK?) at least compiles without error, and I'd rather have some actual tests. If it'll make it easier to collaborate I'll create a branch that y'all can PR against.

If neither of you have time to actually write the tests for the entire wasm community then, if you point me at some things that you're using, that will help me get started.

Thanks for the work and for everyone caring that everything still works!

@jjpe
Copy link
Contributor Author

jjpe commented Nov 27, 2018

The PR is in fact a 2-parter:

  1. Absorb the time crate. This is meant purely as a refactoring i.e. what worked before should work now and vice versa.

  2. The initial wasm32 code. These are just 2 constructors that are conditionally compiled if wasm32 is detected as arch.

As for what I've used so far to test, that would be a 20 KSLOC proprietary library that uses dates and times a lot, and must be able to target both native (with std) and wasm32.

While I'm perfectly fine with adding tests in and of itself, I'm not sure what actually can be done: the new constructors would never execute on native hosts, and in fact chances are it would not even compile there. I could gate the new tests behind the wasm32 arch cfg attribute, but then they'd never be executed during normal test runs.

Got any ideas to solve that?

@quodlibetor
Copy link
Contributor

There are two categories of test I would like to see:

  • compile-pass tests-- just compiling chrono with --target=wasm32-unknown-unknown and --target=wasm32-unknown-emscripten
  • a javascript integration test/script: that imports chrono or some chrono wrapper that ensures that we can actually create a datetime, and with which we just run node ci/wasmtest.js , and which works with with each target.

Does that seem like it would catch major breakage?

I would also prefer to not cause the no-clock use-case to suddenly generate much larger compiled objects, and I don't want to break wasm32-unknown-unknown if somebody else implements it.

@David-OConnor
Copy link

David-OConnor commented May 5, 2019

What's the status of this? It would be nice to have Chrono work on wasm. Alternatively, are there any DT crates that work on wasm?

Seems like this is worth being fixed, tests or not... The current version's failing as-is!

@jasonwilliams
Copy link

Does anyone know what's going on with this?
@jjpe are you able to rebase?

@evq evq mentioned this pull request Aug 16, 2019
quodlibetor added a commit that referenced this pull request Aug 31, 2019
@quodlibetor
Copy link
Contributor

This feature has been released via 331. Thanks for everyone's work!

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.

7 participants