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

use 'chrono' instead of 'time' crate #98

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

astraw
Copy link

@astraw astraw commented Mar 17, 2024

Implements conversion back and forth between
chrono::DateTime and NtpTimestamp and a couple
tests.

This works around
RUSTSEC-2020-0071 by removing the time 0.1 dependency and using
chrono instead.

Implements conversion back and forth between
chrono::DateTime and NtpTimestamp and a couple
tests.

This works around
[RUSTSEC-2020-0071](https://rustsec.org/advisories/RUSTSEC-2020-0071)
by removing the `time` 0.1 dependency and using
`chrono` instead.
Copy link
Owner

@scottlamb scottlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the thorough tests!

I picked time because I wanted a lightweight library for formatting the time in debug output, and in the particular application I use retina in I don't care about that security advisory (I know the environment never changes in non-test code), but you're right: this should be a general-purpose library so it shouldn't have security footguns like that. So I'm sold on switching away from time.

Two things though:

  • I'm less sold on having the time library we use be part of the interface (the new conversions to/fro chrono::DateTime<TZ>. I'm not in love with chrono's API and suspect it will change a bit in the future. I don't particularly want to bump retina's major version every time. Could we maybe compromise on something like UNIX_EPOCH.wrapping_add_secs_and_nanos(secs: u64, nanos: u32) that does the awkward rounding for you but doesn't expose that dependency? (Roughly inspired by https://doc.rust-lang.org/std/time/struct.Duration.html#method.new)
  • Can you commit the Cargo.lock changes? Otherwise CI won't be deterministic, ability to build older versions may bitrot over time, and folks will get a dirty working copy as soon as they run any cargo operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants