guest_emulation_device: move to jiff crate #1028
Conversation
This patch migrates some time crate functions to jiff.
|
@microsoft-github-policy-service agree |
|
We do want to do a full migration, but taking partial steps towards that goal is still goodness. Thanks for submitting this! |
Got it, I will adjust the PR comment then. |
jstarks
changed the title
jstarks
changed the title
Specifically, that you can't get any units bigger than hours *by default*. We also fix what was probably a copy-and-paste error in the `Sub` trait implementation docs for `Zoned`. (I probably copied it from `jiff::civil::DateTime`, where it can return days by default.) And we apply similar clarifications to the `jiff::civil::DateTime` docs as well. Ref microsoft/openvmm#1028 (comment)
Specifically, that you can't get any units bigger than hours *by default*. We also fix what was probably a copy-and-paste error in the `Sub` trait implementation docs for `Zoned`. (I probably copied it from `jiff::civil::DateTime`, where it can return days by default.) And we apply similar clarifications to the `jiff::civil::DateTime` docs as well. Ref microsoft/openvmm#1028 (comment)
|
The aarch64 unit tests are a known spurious failure we're tracking, but the vmm-test failures look legitimate: |
|
|
||
| fn handle_time(&mut self) -> Result<(), Error> { | ||
| const WINDOWS_EPOCH: time::OffsetDateTime = time::macros::datetime!(1601-01-01 0:00 UTC); | ||
| const WINDOWS_EPOCH: DateTime = date(1601, 1, 1).at(0, 0, 0, 0); |
There was a problem hiding this comment.
You could do const WINDOWS_EPOCH: Timestamp = Timestamp::constant(-11644473600, 0); here. Then you can avoid doing the to_zoned dance below.
I think I'd be open to adding this constant to Jiff as well.
There was a problem hiding this comment.
I think we wouldn't want to take that as a hardcoded value unless it was upstreamed, we don't want to be responsible for validating that number haha
There was a problem hiding this comment.
That's fair enough. Yeah otherwise the to_zoned() with the unwrap() is a little unfortunate here. I'll noodle on that one. But it's correct. (The expect() can never fail in this case.)
There was a problem hiding this comment.
I'm OK with taking this constant with a unit test. But I think we can do this as a followup change, when we convert the other side of this protocol to jiff, since we use the same constant in that code. We can just define this centrally in the protocol crate used between these two components.
(This will require a little more refactoring, since the GET crate currently exposes this time as the raw i64 instead of interpreting it as a timestamp for its callers, and the callers are the ones with the epoch definitions and conversions. We can centralize that; I am OK with having a jiff::Timestamp in the public API of this internal crate.)
|
I'll leave this for @jstarks to take one more pass on, but this looks great to me. Thanks again! |
|
Cool, happy to help. Let me know if you want me to squash my commits. |
|
That's alright, we do squash merges, so github will take care of that. |
|
Pushed a small formatting change. Thanks again for the change. |
4 participants
This patch migrates some time crate functions to jiff.
I didn't know if the issue describes a wholesale migration or just the linked PR. If it is a full migration, I can mark this as WIP.
This is part of #238.