wasi-clocks icon indicating copy to clipboard operation
wasi-clocks copied to clipboard

Add timezone back in.

Open cdmurph32 opened this issue 1 year ago • 4 comments

Update README with markdown generation step.

cdmurph32 avatar Feb 28 '24 17:02 cdmurph32

Awesome! Can you also bump the clocks package version to 0.2.1-draft as part of this PR?

pchickey avatar Feb 28 '24 18:02 pchickey

Sounds good.

cdmurph32 avatar Feb 28 '24 19:02 cdmurph32

Updating to wit-abi-up-to-date@v19 in .github/workflows/main.yml should update CI to the latest wit-bindgen.

sunfishcode avatar Feb 28 '24 19:02 sunfishcode

If there aren't any other proposals for 0.2.1, I think the -draft should be removed before this is merged. It needs to be removed before my wasmtime PR. Which in turn needs to be merged (among other things) before the iana-time-zone PR.

cdmurph32 avatar Mar 09 '24 18:03 cdmurph32

I've filed https://github.com/cdmurph32/wasi-clocks/pull/1 to add @unstable feature gate support to this PR. That should give us the ability to merge this into wasi:clocks as a feature which is independently tracked through the phases process. I've also filed https://github.com/WebAssembly/WASI/pull/605 to begin the process of tracking this extension, as well as reserve a canonical feature name (clocks-timezone).

yoshuawuyts avatar May 29 '24 16:05 yoshuawuyts

Thanks @yoshuawuyts . I've merged that.

cdmurph32 avatar May 29 '24 17:05 cdmurph32

@cdmurph32 oh heh, one more thing: there is still a merge conflict on this PR. Could you please rebase it on the main branch? I'm hoping that's the last bit left to get this to a state where it can be merged.

yoshuawuyts avatar May 30 '24 20:05 yoshuawuyts

Do we need to update wit-bindgen?

wit-bindgen markdown wit --html-in-md
Error: failed to resolve directory while parsing WIT for path [wit]

Caused by:
    0: this type is not gated by a feature but its interface is gated by a feature
    1: found a reference to a interface which is excluded due to its feature not being activated
            --> wit/timezone.wit:5:21
             |
           5 |     use wall-clock.{datetime};
             |                     ^-------

cdmurph32 avatar May 30 '24 21:05 cdmurph32

Oh right, yeah — wit-bindgen has not been updated yet with support for the new gates notation. We probably need to cut a release of wasm-tools for that first too.

yoshuawuyts avatar May 30 '24 22:05 yoshuawuyts

wasm-tools and wit-bindgen are released, and I've now bumped their versions in wit-abi-up-to-date v20. So now, it should be sufficient to just bump the wit-abi-up-to-date version to v20 in .github/workflows/main.yml.

sunfishcode avatar May 30 '24 22:05 sunfishcode

Looks like the version of wit-bindgen-cli in the test needs to be updated.

cdmurph32 avatar May 31 '24 15:05 cdmurph32

I've filed https://github.com/WebAssembly/wasi-clocks/pull/67 to update CI, which brings in https://github.com/WebAssembly/wit-abi-up-to-date/pull/26 which was published as part of WebAssembly/wit-abi-up-to-date@v20. I think if we merge that, and then re-run CI here, we should be good.

yoshuawuyts avatar May 31 '24 16:05 yoshuawuyts

#67 is now merged, so now this should just need a rebase.

sunfishcode avatar May 31 '24 21:05 sunfishcode

CI won't pass until we can pass features to wit-abi-up-to-date.

 wit-bindgen markdown wit --check --html-in-md --features clocks-timezone

I'm thinking the github action would need something like this:

    - uses: WebAssembly/wit-abi-up-to-date@v13
      with:
        wit-abi-tag: wit-abi-0.12.0
      inputs:
        features: 
          - clocks-timezone

And wit-abi-up-to-date would need to add --features={{join .inputs.Features ","}} I'm not that familiar with Github actions. How would I test this locally? Is WebAssembly/wit-abi-up-to-date a Docker container?

cdmurph32 avatar Jun 02 '24 00:06 cdmurph32

With wit-abi-up-to-date v21, it should be possible to pass features to the script.

sunfishcode avatar Jun 13 '24 19:06 sunfishcode

Ok, we've now got everything sorted out here. Thanks for all your patience here!

sunfishcode avatar Jun 14 '24 13:06 sunfishcode

Thank you @sunfishcode and @yoshuawuyts !

cdmurph32 avatar Jun 14 '24 14:06 cdmurph32

Hi - I'm one of the champions of the ECMAScript Temporal proposal that will add a richer and timezone-aware set of date/time types to JS engines. @littledan suggested that I take a look at the WASI timezone proposal.

I had a few questions:

  • Is name intended to be a localized string suitable for end users, like "PDT"? Or a software-friendly identifier from the IANA Time Zone Database? Or something else? I'd strongly suggest that the latter is what platforms should offer, because strings like "PDT" are not standardized so it's hard to use them in software, other than for displaying to end users. And if you're intending to use them to display to end users, then those strings should be localized.
  • Is there a reason you chose seconds as opposed to a more fine-grained unit for the time zone offset? I'm asking because there are historical time zones with sub-second offsets, and because in ECMAScript we use nanoseconds to represent time zone offsets.
  • In other contexts, a simple "is DST" boolean has had some challenges when applied to some corner cases, e.g. https://github.com/moment/moment-timezone/issues/968. What are the use cases that you're looking to support with this boolean?
  • What kind of interaction (if any) would you expect to have with Temporal and with ECMAScript overall? Should WASI and ECMAScript date/time types be round-trippable?

cc @ptomato @sffc

justingrant avatar Jun 17 '24 21:06 justingrant

@justingrant I was intending to use the iana-time-zone crate for the wasmtime implementation. Do you have any concerns with that crate?

https://github.com/bytecodealliance/wasmtime/pull/6899

cdmurph32 avatar Jun 18 '24 15:06 cdmurph32

@cdmurph32 - I think you'll want @sffc to weigh in here. He's both a Rust expert and a localization guru, and both may be involved in your question above.

Also, is the reason you're not using ICU4X (the Rust implementation of ICU4C/ICU4J) that WASI is intentionally minimal in size and you don't need the full capabilities of ICU? I assume that ICU4X does everything that iana-time-zone does, and much more. @sffc is one of the folks driving ICU4X.

Also, is the system time zone the only time zone that WASI is worried about? There's no need to be able to calculate the offset for a different time zone?

Finally, where are you getting your IANA Time Zone Database data from in order to calculate the offset for the zone at that instant?

justingrant avatar Jun 18 '24 22:06 justingrant

Thanks for the feedback @justingrant! - I think it's clear we have room to improve, and we should figure out what the right steps forward are. To that effect I've filed https://github.com/WebAssembly/wasi-clocks/issues/69 to continue our conversation about how we can fix the issues in the current design. Let me know if anything is missing there!

yoshuawuyts avatar Jun 20 '24 15:06 yoshuawuyts