-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add timezone host functions. #6899
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
base: main
Are you sure you want to change the base?
Conversation
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasi", "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
2d847a0 to
d4fe100
Compare
7e060ad to
73fdbdc
Compare
33a55d0 to
ac03f95
Compare
Allow clocks-timezone feature to be enabled.
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not expert on timezones or clocks so I can't review the APIs as-is, but despite that I've still got a few questions about this:
- Does it still make sense to use
chronoandchrono-tz? Since this PR was opened I thinkjiffwas released which might be more modern. - Does
chronoorchrono-tzhave any sort of system dependency on something like a timezone database? If so, are there any consequences to that? If not should it? Or will we need to update frequently? - What's the build/size impact of including
chronoandchrono-tz? Adding 10M to the executable? 10K? - We're generally pretty conservative with the defaults of a WASI program, so it might make more sense to default to UTC instead of the local timezone maybe?
- This PR has been pretty inactive for ~2 years and now mergable without much discussion here. That's understandable though if the discussion happened upstream. Do you have links and/or confirmation for others that it's suitable to add this at this time? (or is there no such body that makes sense to confirm this decision?)
|
This PR has been delayed for the past 2 years by the following discussions: Recently it was decided that the result of these discussions will land in 0.3. Therefore this PR can move ahead for 0.2. The 0.3 wit files will be different for all 3 interfaces (wall-clock, monotonic-clock, timezone). Wasmtime releases should use the latest version of whichever crate is providing the IANA timezone database, just as any other software would. There are updates every few, but they are typically driven by geopolitics and are not on a predictable cadence. I will try out jiff as a replacement and report the difference in build size. |
|
Forgive me if these are naive questions, but I haven't been following this proposal closely in the interim years. Do you have a link to a decision somewhere of "let's ship unstable APIs in 0.2"? I'm a bit surprised by that if the APIs are going to be entirely different in 0.3 so I'm curious to read up on the rationale.
Do you have a sense for how other software does? For example does most software bundle the database? Use an external database? (I'm new to timezones in this regard). Basically is Wasmtime doing the "standard thing" here? Or do we know if
To clarify I'm mostly just curious if it was considered. I do not know how to evaluate tradeoffs so I'm relying on expert advice in this area. I, myself, am not equipped to render judgment on a |
|
Looks like https://docs.rs/jiff/latest/jiff/_documentation/design/index.html#chrono and https://docs.rs/jiff/latest/jiff/_documentation/comparison/index.html#chrono-v0438 give a pretty good overview of differences between From my perspective, since
I'd prefer using Since the interface is fairly small and functionality would be largely reused for e.g. p3 or any potential p2 |
| pub struct Timezone { | ||
| // The underlying system timezone. | ||
| timezone: cap_time_ext::Timezone, | ||
| } | ||
|
|
||
| impl Default for Timezone { | ||
| fn default() -> Self { | ||
| Self::new(ambient_authority()) | ||
| } | ||
| } | ||
|
|
||
| impl Timezone { | ||
| pub fn new(ambient_authority: AmbientAuthority) -> Self { | ||
| Self { | ||
| timezone: cap_time_ext::Timezone::new(ambient_authority), | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this interface would have been more useful if embedders were able to provide an arbitrary timezone to use with UTC default.
There are many ways to do that, but perhaps we could:
- store
jiff::tz::TimeZonehttps://docs.rs/jiff/latest/jiff/tz/struct.TimeZone.html in the struct - provide
TryFrom<cap_time_ext::Timezone> for Timezone - provide
From<jiff::tz::Timezone> for Timezone
If we had that, we could have a Default implemented for Timezone, which would default to UTC.
We could then:
- rename
newto something likenew_hostand make it fallible - add
new_utc, which acts the same asDefault - maybe implement
FromStrforTimezonein terms of https://docs.rs/jiff/latest/jiff/tz/struct.TimeZone.html#method.get ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should replace iana_time_zone with jiff in cap-std-ext?
https://github.com/bytecodealliance/cap-std/blob/main/cap-time-ext/src/timezone.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn timezone_from_duration(&self, datetime: Duration) -> Option<TimezoneDisplay> {
let tz = self.timezone.timezone().ok()?;
let timestamp = Timestamp::from_second(datetime.as_secs() as i64).ok()?;
let localtime = tz.to_offset_info(timestamp);
let utc_offset = localtime.offset().seconds();
let name = tz.iana_name().unwrap_or("UTC").to_string();
let in_daylight_saving_time = jiff::tz::Dst::Yes == localtime.dst();
Some(TimezoneDisplay {
utc_offset,
name,
in_daylight_saving_time,
})
}If I replace iana_time_zone with jiff in cap-std-ext we could have something like this. Jiff automatically returns UTC if the timezone cannot be determined. Does this look better?
Pros: No more linear search
Cons: cap-std-ext::Timezone now returns a giff::Timezone instead of a string. Not really std behavior.
If that Con is a deal breaker, I could keep the return type a string and convert the string back into a jiff timezone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's not necessarily a requirement to change cap-std bits, it's fine to use jiff directly here in Wasmtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to wasmtime-wasi jiff directly instead of going through cap-std-ext, I don't think the indirection through that additional crate is really getting us much in this case. The user can provide.a jiff::Timezone to the WasiCtxBuilder and if they don't it should default to UTC. We can then have a wasmtime-cli flag for setting a time zone, or "auto" which allows jiff to make its best effort to figure one out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd agree with @rvolosatovs that the default time zone should be UTC. I don't doubt that many embedders will want to change this but Wasmtime's default stance has been one to sandbox everything and picking a deterministic default feels more sand-box-y to me than inferring based on the host system. For embedders that want to use the native time zone though I'd want to make sure we can easily enable that with a single call on WasiCtxBuilder, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to default to UTC, then I don't see any role for wasmtime at all. There is no host functionality to be provided. Happy to be wrong. I just don't understand. I thought the point of this PR was to provide the host timezone to the guest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear you might be misunderstanding me, but to clarify I'd think of this like environment variables. By default wasmtime doesn't give any environment variables to the guest, but the WasiCtxBuilder structure has methods to pass environment variables. The default behavior of "nothing" doesn't mean that the implementation of environment variabls is pointless it's just the default behavior.
I'm thinking the same thing for time zones. By default it's UTC, but embedders can easily configure "use this time zone instead" where "this" could be something specific or the system's local time zone. I'm not sure why this behavior would lead to the implementation having no role in Wasmtime. Wasmtime is still providing functionality, time zones and information, and embedders can configure which time zone is reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably be easily smoothed over in a quick chat, but I'll try to articulate my understanding as best I can here. Today, there is nothing technically stopping someone from using timezones in their component and setting the timezone to UTC or whatever timezone they want. If they want to enable host timezone access, they would need something like what this PR provides.
If we want that functionality not to be enabled by default then that's fine. Perhaps that is what you mean.
Where I'm coming from people expect their Wasm applications to work just like their native code. For C2PA, all of our tests run with -S cli -S http --dir .. Too many WASI specific code paths and it just isn't worth the hassle to maintain.
If --dir isn't passed and and file access is denied, then the code panics. Likewise if people don't want the system timezone, then they shouldn't pass the feature flag. If they do want it, then it should just work as like the native code, which is their expectation.
To put it another way JiffTimeZone::try_system() should behave exactly the same on WASI as it does for any other target if the feature is enabled, if the feature is not enabled then it should return an error. This is the expected behavior if the system timezone is unavailable. There is no other way for large projects with numerous dependencies to be maintainable.
Apologies if you are saying the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why we'd need different code paths?
For the CLI you'd pass something like --inherit-timezone to use the timezone of the host or --timezone $mytz to use some custom one. If you don't specify any - it runs in UTC by default.
In case of an embedding you'd use analogous options on the WasiCtxBuilder
If this functionality does not address your use case, I'd really like to know more about it
The ambient authority is unnecessary for timezone. The Timezone struct is a wrapper for a jiff::tz::timezone
No apology necessary. Other than this WASI and wasmtime work, I have no particular experience with timezones or clocks either. On the other hand, the proposed changes for clocks in 0.3 rename monotonic and wall-clock. I'm not sure exactly how they would work with the code as presently constructed. Timezone will probably also change.
Yes, both chrono and jiff come with their own timezone databases. This is the standard as far as I can tell. If a new timezone is created after the database in use and the system returns that new timezone, the jiff::tz::Timezone will be "TimeZone::unknown()", and as my PR is currently written, the timezone provided by wasmtime will fallback to
As @rvolosatovs mentioned, |
|
@cdmurph32 do you have a link to the discussion to ship this as unstable in 0.2? I understand it's all off-by-default, but I'd still like to read the discussion. A 1M size increase isn't the end of the world but that's also pretty chunky for an off-by-default API too... |
That's fine. We can wait for 0.3. |
Issue:
#4274
WASI Clocks proposal:
WebAssembly/wasi-clocks#48
Relate cap-std PR:
bytecodealliance/cap-std#327