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

Port from chrono to time 0.3 #63

Closed
wants to merge 1 commit into from

Conversation

Cogitri
Copy link
Contributor

@Cogitri Cogitri commented Dec 24, 2021

Chrono has been unmaintained for some time now and has had open CVEs
for a while now that aren't getting fixed. See chronotope/chrono#602

Chrono has been unmaintained for some time now and has had open CVEs
for a while now that aren't getting fixed. See chronotope/chrono#602
@Cogitri Cogitri closed this Jan 24, 2022
@shssoichiro
Copy link
Owner

Sorry, I really should have gotten to this a while back. Is it still the case that chrono should be phased out?

@Cogitri
Copy link
Contributor Author

Cogitri commented Jan 25, 2022

Ah, no worries. I think chrono maintenance has picked up again (or at least there are some commits on master again) and the time crate has a problem: To avoid the unsoundness of chrono it just doesn’t do local time unless an additional config flag is specified:

One pseudo-feature flag that is only available to end users is the unsound_local_offset cfg. As the name indicates, using the feature is unsound, and may cause unexpected segmentation faults. Unlike other flags, this is deliberately only available to end users; this is to ensure that a user doesn’t have unsound behavior without knowing it. To enable this behavior, you must use RUSTFLAGS="--cfg unsound_local_offset" cargo build or similar. Note: This flag is not tested anywhere, including in the regular test of the powerset of all feature flags. Use at your own risk. Without this flag, any method that requires the local offset will return the Err variant.

So I’m not sure if time really fills the gap chrono fills.
https://docs.rs/time/latest/time/

@complexspaces
Copy link
Contributor

@Cogitri Would you consider re-opening this PR, but without support for local timezones? Chrono is receiving infrequent commits again, but it has been reported that the author is still refusing to update to newer time versions or break the API.

I would like to make the argument that zxcvbn's use case doesn't justify the downsides of continuing to depend on chrono when all we need is precision to the year granularity. This crate would instead use UTC. IIUC, the worst case would be that there is a few hours every year where different clients in different timezones may get differing results. zxcvbn is usually used in client-side UIs, so this seems acceptable at a glance.

While it is not exactly the same use case, tracing has also replaced chrono for prior art.

@Cogitri
Copy link
Contributor Author

Cogitri commented Mar 11, 2022

Sure, if that’s desirable I can re-open this :)

@complexspaces
Copy link
Contributor

I see a 👍 from @shssoichiro, but granted I don't know how much of a desirability marker that is :)

@shssoichiro
Copy link
Owner

I'd be willing to merge this, given the patterns in chrono's maintenance and that time is lighter weight in general.

@Cogitri
Copy link
Contributor Author

Cogitri commented Mar 15, 2022

Apparently GH doesn't allow re-opening PRs after force-pushing, so I opened #64

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.

3 participants