Switch from `chrono` to `time` #61

connec · 2021-10-29T14:57:20Z

I'm interested in seeing rustls/rcgen#65 resolved, so thought I'd take a crack at replacing chrono with time here to see if there was appetite to merge such a change.

Overall the change was fairly mechanical, with times API actually being a bit neater in the end, imo. The main limitation seems to be regarding leap seconds. For now I've gone for simply ignoring them. Another option would be to record it in the UTCTime/GeneralizedTime structs themselves, so that it can be round-tripped in parsing/formatting (but would still not be recorded in the underlying OffsetDateTime, exposed through the datetime() method).

Since chrono types were used in the API, this is a breaking API change. Since a breaking change was already necessary, I also changed the feature to time.

I've tried to preserve the existing formatting, however some of it may have drifted to default rustfmt style. Let me know if anything needs changed.

There is unsoundness in `chrono` (https://rustsec.org/advisories/RUSTSEC-2020-0159), and although we don't use the affected APIs, the advisory still fires. `time` ≥3 does not have this unsoundness, and supports almost‡ all the necessary APIs, so we can quite simply swap it out. Since the `chrono` types are used in some APIs, those signatures have had to change, making this a breaking change. Since it's already a breaking change, the feature is also renamed from `chrono` to `time`. ‡ `time` does not support leap seconds in their representation or parsing. In this commit, we simply ignore leap seconds, which also constitutes a breaking change since values with leap seconds will parse differently (they parse to the previous second).

connec · 2021-10-29T21:35:54Z

So it looks like time ≥ 3 is not compatible with Rust 1.36.0. Furthermore time has quite an aggressive policy regarding MSRV:

The time crate is guaranteed to compile with any release of rustc from the past six months. Optional feature flags that enable interoperability with third-party crates (e.g. rand) follow the policy of that crate if stricter.

Additionally, one of the change notes says:

Per the policy in the README, this may be bumped within the 0.3 series without being a breaking change.

The current MSRV for the latest time (0.3.4) appears to be 1.51. For the 0.3 line the lowest MSRV is 1.48. It looks like time ≥ 0.2.23 is also advisory free, and it seems that would let the MSRV drop to 1.32.0.

I don't see any documentation for MSRV for this repo.

The two sensible options seem to be:

Drop to time = "0.2.27" and retain the 1.36.0 MSRV, rather than adopt the comparatively fast-paced MSRV for time ≥ 3. This would perhaps be the best option for the stability of this crate, at the expense of potentially missing important updates to time (plus, having crate ambiguity w/ use of the latest version).
Adopt a similar policy as time regarding MSRV wrt to optional features, i.e. "Optional feature flags that enable interoperability with third-party crates (e.g. rand) follow the policy of that crate if stricter". This seems like a nice compromise, and is perhaps what people looking to interoperate with time would prefer, rather than having to pin their own dependency to 0.2.27. This may require periodic CI updates to account for the inexorable march of time's MSRV.

Some optional dependencies, for example `time`, have a more strict MSRV than this crate. Rather than having to upgrade our MSRV or pin outdated versions of dependencies, we can instead explicitly exclude optional features from the MSRV. The test CI workflow has been updated to not run tests with features on MSRV.

connec · 2021-10-29T22:17:38Z

I went ahead and added a commit taking the second approach (caveat the documented MSRV with "Optional feature flags that enable interoperability with third-party crates (e.g. time) follow the policy of that crate if stricter"), and updated the CI workflow to not test with features on MSRV.

Let me know if you'd prefer another approach.

est31

Thanks for filing this PR! Some remarks

the docs of the from_datetime and from_datetime_opt and datetime functions should be changed to mention OffsetDateTime instead of datetime.
regarding the MSRV, I'm open towards increasing it to as recent as 1.51 to match the time crate, but then ideally yasna.rs itself also uses at least one feature that has been introduced by the respective version. The changes regarding MSRV from this PR are fine, just saying that this can be the subject of future PRs. Increasing the MSRV to older releases than 1.51 is also possible, there just needs to be a feature that is worth the change.
the biggest issue I have with the PR is the missing handling of leap seconds. They still exist and even single seconds are treated seriously in the security domain. I'm not sure I can accept a PR that switches to a library that ignores leap seconds, but happy to hear input on this.

src/models/time.rs

est31 · 2021-10-30T10:33:38Z

src/models/time.rs

@@ -671,7 +644,7 @@ fn test_generalized_time_parse() {

    let datetime =
        GeneralizedTime::parse(b"19990101085960.1234+0900").unwrap();
-    assert_eq!(&datetime.to_string(), "19981231235960.1234Z");
+    assert_eq!(&datetime.to_string(), "19981231235959.1234Z");


This is definitely not great.

connec · 2021-10-30T10:56:44Z

Thanks for the feedback!

I'll see what can be done regarding leap second handling. I didn't find much when searching the time repo for leap seconds but hopefully something will be possible.

Good catch about the docs, I should review them more carefully.

Regarding the MSRV, I think that makes sense. I'm inclined to leave it as-is in this PR, sadly not sure I'm able to go down the route of a more general upgrade.

est31 · 2021-10-30T10:57:57Z

Re the leap seconds, I've opened an issue at: time-rs/time#377 . It seems that the time maintainers want proper handling instead of the improper one that the chrono crate has.

est31

So I've thought about it a little, and it's definitely not great that round trip ability is hurt by this. Should the need arise (users complain or such), we can implement our own datatype and provide converters to the time formst.

I really hope future versions of the time crate support leap seconds and if so, we can enable them. Could you add TODOs to the test as well as to the place where we are ignoring leap seconds?

src/models/time.rs

This is a better match for the `time` terminology.

This is a bit of hack until `time` has built-in support for leap seconds. When we detect a leap second in `GeneralizedTime`, we set the `is_leap_second` flag to true. When serializing to bytes, we check the flag and bump the seconds to 60 if it's set. A debug assert ensures we'll notice if the flag is set incorrectly.

connec · 2021-10-30T12:31:06Z

I've pushed a couple of commits, one fixing the docs and another with a hack for round-tripping leap seconds. The hack brings back the previous test assertion, and I updated the documentation for GeneralizedTime::datetime to indicate that leap seconds are discarded in the returned OffsetDateTime.

If you'd prefer I can drop the hack and just document as TODO – I started with this before I noticed your last comment 😄

est31

Thanks!

est31 requested changes Oct 30, 2021

View reviewed changes

est31 reviewed Oct 30, 2021

View reviewed changes

src/models/time.rs Outdated Show resolved Hide resolved

connec added 2 commits October 30, 2021 13:15

Update datetime references in docs to OffsetDateTime

9532b34

This is a better match for the `time` terminology.

est31 approved these changes Oct 30, 2021

View reviewed changes

est31 merged commit 8d7ff83 into qnighy:master Oct 30, 2021

connec deleted the chrono-to-time branch October 30, 2021 13:55

connec mentioned this pull request Nov 1, 2021

Switch from chrono to time rustls/rcgen#66

Merged

Ralith mentioned this pull request Nov 21, 2021

New release w/o chrono dep #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from `chrono` to `time` #61

Switch from `chrono` to `time` #61

connec commented Oct 29, 2021

connec commented Oct 29, 2021 •

edited

Loading

connec commented Oct 29, 2021

est31 left a comment

est31 Oct 30, 2021

connec commented Oct 30, 2021

est31 commented Oct 30, 2021 •

edited

Loading

est31 left a comment

connec commented Oct 30, 2021 •

edited

Loading

est31 left a comment

Switch from chrono to time #61

Switch from chrono to time #61

Conversation

connec commented Oct 29, 2021

connec commented Oct 29, 2021 • edited Loading

connec commented Oct 29, 2021

est31 left a comment

Choose a reason for hiding this comment

est31 Oct 30, 2021

Choose a reason for hiding this comment

connec commented Oct 30, 2021

est31 commented Oct 30, 2021 • edited Loading

est31 left a comment

Choose a reason for hiding this comment

connec commented Oct 30, 2021 • edited Loading

est31 left a comment

Choose a reason for hiding this comment

Switch from `chrono` to `time` #61

Switch from `chrono` to `time` #61

connec commented Oct 29, 2021 •

edited

Loading

est31 commented Oct 30, 2021 •

edited

Loading

connec commented Oct 30, 2021 •

edited

Loading