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

TimeZone::system(): support WASM as a platform #56

Closed
sharkdp opened this issue Jul 29, 2024 · 9 comments · Fixed by #58
Closed

TimeZone::system(): support WASM as a platform #56

sharkdp opened this issue Jul 29, 2024 · 9 comments · Fixed by #58
Labels
enhancement New feature or request platform Issues related to platform support.

Comments

@sharkdp
Copy link
Sponsor Contributor

sharkdp commented Jul 29, 2024

While porting Numbat to jiff (sharkdp/numbat#511), I noticed that jiff's timezone detection doesn't work on WASM (currently panics with "time not implemented on this platform"). I am currently using iana-time-zone which supports timezone detection on wasm32 out of the box. I'm opening this ticket since you've mentioned that jiff attempts to support more platforms in the future.

Here is how iana-time-zone does the detection for Browser/Node based environments: strawlab/iana-time-zone#38. They basically run

Intl.DateTimeFormat().resolvedOptions().timeZone

using js_sys. This is supported in all major browsers.

@BurntSushi BurntSushi added the enhancement New feature or request label Jul 29, 2024
@BurntSushi
Copy link
Owner

I agree support for WASM should be added here. Although I think this is not necessarily "wasm support," but rather, "web browser support." For contexts outside of web browsers, I don't imagine this form of detection (running Javascript code) is what one would want. So I think we'll need to explore in what scenarios this sort of detection should be enabled.

I noticed that jiff's timezone detection doesn't work on WASM (currently panics with "time not implemented on this platform")

Hmmm. Time zone detection should never panic, even if it's unsupported. The panic message you're getting suggests that SystemTime::now is what's panicking? Or am I misreading?

@sharkdp
Copy link
Sponsor Contributor Author

sharkdp commented Jul 29, 2024

The panic message you're getting suggests that SystemTime::now is what's panicking? Or am I misreading?

No, you're right. I mixed up two problems that happened at the same time. With chrono, I could call chrono::Local::now() and it worked on WASM (probably this? https://github.com/chronotope/chrono/blob/1eb3174428d8745dd06b645aefb3403445d74d06/src/offset/utc.rs#L101-L111). I replaced that with Zoned::now() and that panics with the message above.

Time zone detection should never panic, even if it's unsupported.

I was wrong above. jiff::TimeZone::system() currently falls back to TimeZone::UTC on WASM.

Although I think this is not necessarily "wasm support," but rather, "web browser support."

I believe it would also work for Node. And I guess it makes sense to let Node handle the platform abstraction in that case. Because if you want to do e.g. Linux/Windows-specific timezone detection from within a Node environment, you would have to go through the JS File-IO/Environment-Variable API. And you would risk getting a different result than other parts of the Node application that might rely on the standard(?) way to retrieve the timezone via Intl.DateTimeFormat.

But yeah, there might be other systems that you want to run your WASM binaries on. Like wasmtime. But it looks like they don't support timezone retrieval just yet? (see WebAssembly/wasi-clocks#69 where you also commented)

@BurntSushi
Copy link
Owner

BurntSushi commented Jul 29, 2024

And I guess it makes sense to let Node handle the platform abstraction in that case. Because if you want to do e.g. Linux/Windows-specific timezone detection from within a Node environment, you would have to go through the JS File-IO/Environment-Variable API. And you would risk getting a different result than other parts of the Node application that might rely on the standard(?) way to retrieve the timezone via Intl.DateTimeFormat.

So the approach Jiff is at least currently taking here is that it should really try to use tzdb for this through the IANA tz identifier. This is why, for example, tzdb is actually bundled into the binary on Windows, because Windows doesn't have a standard installation location of tzdb. And Window's default time zone handling is pretty whacky.

Porting this decision over to the web browser case, implies, I think, that Jiff embeds tzdb for use in the browser. But it doesn't need to do this in all cases for wasm32. For example, today, Jiff works in wasmtime and can even access /usr/share/zoneinfo (assuming you pass the necessary flags). So it should only be for some cases. But embedding the entire tzdb into the web browser is maybe not great, but probably not the end of the world. This is partially why I want #20, which should make the total embedded size much smaller (perhaps by an order of magnitude?).

The main problem with using "platform" APIs for this is that there generally isn't a universal way of looking up time zone rules for an IANA time zone identifier. It just doesn't exist, which is why Jiff owns the tzdb interaction. What is usually available for the platform is "get me the current local time and its corresponding offset." But that is extremely limited. It doesn't expose the actual time zone identifier and thus provides no way to do DST safe arithmetic. Moreover, it inhibits lossless serialization because you can't encode the time zone identifier because you don't have it.

OK, so this issue is somewhat tangled up with a few different things:

  1. Jiff currently panics on at least some WASM targets because SystemTime::now() panics. But it's definitely not all WASM targets.
  2. In web browsers, Jiff doesn't use the platform API for determining the IANA time zone identifier, but it seems like there is a way to do that using Intl.DateTimeFormat().resolvedOptions().timeZone.
  3. In web browsers, there is no universal way of accessing tzdb for arbitrary time zones. So Jiff must embed tzdb into the binary if one wants the full "power" of Jiff.
  4. There is likely a tweener state here that Jiff doesn't quite service today, which is the ability to create a Zoned via Zoned::now() using the local offset but not the IANA time zone identifier. The omission of this state is actually pretty intentional because it creates a giant footgun by giving you what you think is a local Zoned datetime, but actually won't do DST safe arithmetic at all. The main problem with not supporting this, I think, is that a lot of datetime libraries have been built up around exactly this sort of thing (I'm thinking of time and chrono here, with chrono providing an opt-out via chrono-tz) that it can make migration difficult. Also, not all use cases necessarily want to do arithmetic on datetimes or even explicitly don't care about DST safe arithmetic, which means people in this category need to bring in tzdb support even though they don't strictly need it.

For (1) and (2), I think this is just blocked on figuring out the right cfg gates and is otherwise straight-forward to fix. I don't have a ton of WASM experience here, so any help on this point would be most appreciated. Otherwise, I'll probably emulate getrandom since I have a lot of confidence that they're getting it correct. I just don't know if the assumptions they make are also appropriate for me to make for Jiff.

For (3), I don't think there's really anything to be done here. There just isn't a platform independent way of accessing the IANA Time Zone Database. It's beyond my power to fix. This can be made better by reducing the embedded database size by finishing the zic compiler support. (A ton of it is done. Jiff has a complete parser for the plain text tzdb format. I just stalled at the part where the rules are interpreted into time zone transitions.)

For (4), a "minimal" way out of this would be to introduce something like jiff::tz::TimeZone::system_fixed which will create a TimeZone::fixed for the offset of the current time. I really do not want to make this a "fallback" in Zoned::now though because of the enormous footgun it would introduce. But it would at least provide a way for folks to do "depend on jiff, disable tzdb integration entirely and still get the local time and offset." But it would require a very explicit opt-in to do it, and I can put warnings on TimeZone::system_fixed.

@sharkdp
Copy link
Sponsor Contributor Author

sharkdp commented Jul 29, 2024

So it should only be for some cases. But embedding the entire tzdb into the web browser is maybe not great, but probably not the end of the world. This is partially why I want #20, which should make the total embedded size much smaller (perhaps by an order of magnitude?).

Anything that would decrease the size of the tzdb (which I want to have embedded in my wasm binary) would be very welcome! It was definitely a concern when we added chrono_tz in Numbat at the time, see sharkdp/numbat#287 (comment). It currently increases the size of the WASM binary by 1 MiB, which is definitely noticeable in the startup time of the web application.

OK, so this issue is somewhat tangled up with a few different things:

Sorry about that. Let me know if I should open a new ticket for the SystemTime::now() problem.

In web browsers, there is no universal way of accessing tzdb for arbitrary time zones. So Jiff must embed tzdb into the binary if one wants the full "power" of Jiff.

Again, if it wasn't clear: I definitely want the full power of jiff 😄

There is likely a tweener state here that Jiff doesn't quite service today, which is the ability to create a Zoned via Zoned::now() using the local offset but not the IANA time zone identifier. The omission of this state is actually pretty intentional because it creates a giant footgun by giving you what you think is a local Zoned datetime, but actually won't do DST safe arithmetic at all.

I can't speak for everyone else, but I'm personally fine with this restriction, because looking at iana-time-zone, it looks like I will, in principle, be able get the actual local timezone (and not just the offset) on all platforms that I am interested in. And of course I don't want bugs in my programs. Those kind of safety considerations are the main decision factor when choosing a datetime library. The absence of unsafe methods is a good thing. It might be fine of course if it comes with enough warnings.

@BurntSushi
Copy link
Owner

Great! I think just keeping this as one issue for now is fine. We can create new issues for finer grained problems after getting over this initial hump.

I think it sounds like your use case lines up pretty well with Jiff then! Hopefully I can figure out how to finish the zic compiler and then I think the embedded size should shrink considerably.

I'm working on WASM support now. I spoke with the WASI and wasmtime folks about how best to do this, and the conclusion was I think to copy what the getrandom crate does. The TL;DR is that you (the application author) will need to enable a js feature for Jiff (incoming) and then everything should just work. iana-time-zone doesn't require opt-in support because it assumes every wasm32 target is used in a web context. Which, as I've learned, is egregiously wrong!

But once you enable js, it should make Zoned::now() work. And it should make TZ detection work (based on web browser APIs). And I will be adding target_family = "wasm" to the list of platforms that automatically get tzdb bundled by default. (But folks can opt out of this if they want.) So I think that will bring everything full circle for the wasm use case here!

@sharkdp
Copy link
Sponsor Contributor Author

sharkdp commented Jul 29, 2024

iana-time-zone doesn't require opt-in support because it assumes every wasm32 target is used in a web context. Which, as I've learned, is egregiously wrong!

Correct. We ran into that as well when we embedded Numbat into an IRC bot that used wasmtime for hot-reloading the Numbat "plugin". We currently patched this on our own (using a feature gate) in Numbat (see sharkdp/numbat#460).

But once you enable js, it should make Zoned::now() work. And it should make TZ detection work (based on web browser APIs). And I will be adding target_family = "wasm" to the list of platforms that automatically get tzdb bundled by default. (But folks can opt out of this if they want.) So I think that will bring everything full circle for the wasm use case here!

Sounds great. If I understand correctly, we would still need to do some feature-switching in Numbat on our own though. Because when we compile Numbat for wasm32-wasi, we should not pass that js feature down to jiff (we would be okay with a UTC fallback for wasm32-wasi.. because that bot runs on a server anyway and the "users" timezone is not at all related to the timezone of the server).

@BurntSushi
Copy link
Owner

BurntSushi commented Jul 29, 2024

Because when we compile Numbat for wasm32-wasi, we should not pass that js feature down to jiff

No you'll be fine! It's a feature that only enables js-sys and wasm-bindgen on wasm{32,64}-unknown-unknown. It has no effect on wasm32-wasi (or, rather wasm32-wasip1 now) targets. I missed this when I first looked at getrandom, but it has the same setup. So basically, you can just enable js and it will do the right thing.

But yeah, as my Zulip chat revealed, there really isn't any way of reliably discovering the current time zone on wasm32-wasip1. It looks like that will become possible in wasm32-wasip2 though: WebAssembly/wasi-clocks#69

@BurntSushi BurntSushi added the platform Issues related to platform support. label Jul 30, 2024
BurntSushi added a commit that referenced this issue Jul 31, 2024
This PR basically makes the `wasm{32,64}-unknown-unknown` targets work
_almost_ out of the box. All you need to do is enable Jiff's new `js`
crate feature. This will cause Jiff to depend on `js-sys` and
`wasm-bindgen`. Jiff will then use Javascript APIs to determine the
current time and time zone.

This PR also includes a new long form guide, `PLATFORM.md`, which
describes Jiff's platform support in one central location. (Most
information is already in Jiff's API docs, but it's scattered in a
variety of places.)

Finally, this adds a `wasm32-unknown-unknown` test to CI courtesy of
`wasm-pack`. It just does a basic sanity check that the current time and
time zone can be retrieved.

Fixes #56
BurntSushi added a commit that referenced this issue Jul 31, 2024
This PR basically makes the `wasm{32,64}-unknown-unknown` targets work
_almost_ out of the box. All you need to do is enable Jiff's new `js`
crate feature. This will cause Jiff to depend on `js-sys` and
`wasm-bindgen`. Jiff will then use Javascript APIs to determine the
current time and time zone.

This PR also includes a new long form guide, `PLATFORM.md`, which
describes Jiff's platform support in one central location. (Most
information is already in Jiff's API docs, but it's scattered in a
variety of places.)

Finally, this adds a `wasm32-unknown-unknown` test to CI courtesy of
`wasm-pack`. It just does a basic sanity check that the current time and
time zone can be retrieved.

Fixes #56
BurntSushi added a commit that referenced this issue Jul 31, 2024
This PR basically makes the `wasm{32,64}-unknown-unknown` targets work
_almost_ out of the box. All you need to do is enable Jiff's new `js`
crate feature. This will cause Jiff to depend on `js-sys` and
`wasm-bindgen`. Jiff will then use Javascript APIs to determine the
current time and time zone.

This PR also includes a new long form guide, `PLATFORM.md`, which
describes Jiff's platform support in one central location. (Most
information is already in Jiff's API docs, but it's scattered in a
variety of places.)

Finally, this adds a `wasm32-unknown-unknown` test to CI courtesy of
`wasm-pack`. It just does a basic sanity check that the current time and
time zone can be retrieved.

Fixes #56
@BurntSushi
Copy link
Owner

This should be fixed in jiff 0.1.3 on crates.io. You'll just want to enable the js crate feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request platform Issues related to platform support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants