Skip to content

Hack around a getting precise time in Windows #231

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

Closed
wants to merge 3 commits into from

Conversation

da-x
Copy link

@da-x da-x commented Mar 10, 2020

This takes the approach from rust-lang/rust#67266 by @dbergman. This may be necessary for the v0.1 branch of time because the widely-used chrono crate still depends on it. When writing logs on Windows, crates such as simplelog which depend on chrono would benefit from a better timestamp. @quodlibetor @Drakulix

@da-x da-x force-pushed the 1.x-precise-time-on-windows branch from 4ec76f8 to 59ae91d Compare March 10, 2020 15:57
@jhpratt
Copy link
Member

jhpratt commented Mar 10, 2020

A few things:

  • CI is failing due to the use of a static atomic
  • I don't have knowledge of Windows APIs. Please comment the code so I can follow what it's doing and why.
  • This code does not appear to be compatible with Windows < 8, per the rust-lang issue.
  • What benefit does this provide, exactly? Time v0.1 is no longer supported, so only major issues are being fixed.

@da-x
Copy link
Author

da-x commented Mar 10, 2020

  • Seems to be failing due to Rust 1.24 compatibility. I'll look into resolving it.
  • Sure, I'll add the comments.
  • It should be compatible with older versions of Windows, as it implements a fallback to the older function if the newer function is not exported from kernel32. The pending PR I've opened for std does something similar but in a cleaner approaching using existing std machinery.
  • The major benefit is the accuracy of a timestamp going down significantly from 10ms to the 1usec area. This is useful for applications that write log files.

@jhpratt
Copy link
Member

jhpratt commented Mar 12, 2020

I'm still not quite sure this is worthy of another release. As I mentioned, 0.1 isn't supported any more. The current limitation of ~10ms is how it's been for a very long time. If this is released in a 0.1.43, there are a number of other things that I have previously closed that people would want to revisit.

Is there anywhere this has caused major issues that can't be worked around?

@jhpratt jhpratt added the C-feature-request Category: a new feature (not already implemented) label Mar 12, 2020
@da-x
Copy link
Author

da-x commented Mar 14, 2020

I guess that a possible workaround is to bake this functionality straight into the chrono crate, and have it not depend on old time, maybe only for Windows. Maybe @quodlibetor can comment?

@jhpratt jhpratt added the C-seeking-input 📣 Category: community input is desired label Mar 17, 2020
@jhpratt jhpratt added the v0.1 label Apr 5, 2020
@jhpratt
Copy link
Member

jhpratt commented Apr 7, 2020

As I decided in #240, I'll be creating one final release for 0.1 at some point in the future. As such, I'll review this PR along with others when I get the chance.

@jhpratt jhpratt added C-keep-open Category: should not be closed due to inactivity and removed C-seeking-input 📣 Category: community input is desired labels Apr 7, 2020
@pfmooney
Copy link

It should be compatible with older versions of Windows, as it implements a fallback to the older function if the newer function is not exported from kernel32. The pending PR I've opened for std does something similar but in a cleaner approaching using existing std machinery.

It would be nice to have a test plan for these older versions of Windows, both with stable rust and 1.21 (if possible).

@jhpratt jhpratt removed the C-keep-open Category: should not be closed due to inactivity label Apr 20, 2020
@github-actions
Copy link

This pull request has not had any activity recently. It will be closed without further activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: a new feature (not already implemented)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants