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

Support dates prior to Unix Epoch on Linux #990

Closed
wants to merge 1 commit into from

Conversation

Byron
Copy link

@Byron Byron commented Jan 15, 2024

This changes the st_(a|m|c)time fields on Linux to i64 from u64 to support times before Unix Epoch.

Before this change, downstream crates would see i64 times that have been cast to u64, which is unexpected as it then represents a time unimaginably far in the future.


Review Notes

There are plenty of tests which explicitly cast these fields to u64 for comparison, or convert them to u64. These tests don't fail as for most current dates, the time fields won't be negative.
It might be confusing to see this done and it's probably better to remove these casts or, if needed, cast to i64 instead. For now I left the PR minimal though and hope you can guide me (or make the changes you need) depending on preferences. After all, I could just approach the issue from the perspective of one Platform with one particular issue, but Rustix supports so many platforms more that I can barely grasp what's right here 😅.

This changes the `st_(a|m|c)time` fields on Linux to
`i64` from `u64` to support times before Unix Epoch.

Before this change, downstream crates would see `i64`
times that have been cast to `u64`, which is unexpected
as it then represents a time unimaginably far in the future.
@joshtriplett
Copy link
Member

Can confirm, this is necessary for any file with a date before the epoch.

AFAICT, this will require a major version bump.

@sunfishcode
Copy link
Member

Thanks for the PR! Yes, this will require a major version bump, which will take some more time. For now, I've posted #999 as a temporary fix that doesn't require a major version bump.

@sunfishcode sunfishcode added the semver bump Issues that will require a semver-incompatible fix label Jan 19, 2024
@Byron
Copy link
Author

Byron commented Jan 19, 2024

Thanks so much for your help with this issue and the non-breaking solution to it! It's very neat :).

Closing this PR seems best as the type-change proposed here can trivially be reproduced once the next major release is being prepared, and thanks to the deprecation notices on these fields introduced in #999 I am sure they can't be missed.

@Byron Byron closed this Jan 19, 2024
@joshtriplett
Copy link
Member

@Byron Up to @sunfishcode, but I suspect it'll help to leave this open so it's easy to find all the proposed semver-bumping changes when preparing the next major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver bump Issues that will require a semver-incompatible fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants