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

Fix CTimeVal definition for platforms where time_t isn't CLong #318

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Apr 30, 2024

One such platform is Debian unstable armhf, which is in the process of transitioning to 64-bit time_t: https://wiki.debian.org/ReleaseGoals/64bit-time

On that platform (as well as any other glibc/musl platform), however, CTimeVal isn't used for anything at all because there are #ifdefs that prefer using utimensat which takes CTimeSpec instead. This fix is therefore quite theoretical, as it is unknown whether there are any platforms actually affected.

Related: #252

@Bodigrim
Copy link
Contributor

Bodigrim commented May 1, 2024

@liskin could you please rebase? Hopefully CI will get better.

One such platform is Debian unstable armhf, which is in the process of
transitioning to 64-bit time_t: https://wiki.debian.org/ReleaseGoals/64bit-time

On that platform (as well as any other glibc/musl platform), however,
CTimeVal isn't used for anything at all because there are #ifdefs that
prefer using `utimensat` which takes CTimeSpec instead. This fix is
therefore quite theoretical, as it is unknown whether there are any
platforms actually affected.

Related: haskell#252
@liskin
Copy link
Contributor Author

liskin commented May 2, 2024

@liskin could you please rebase? Hopefully CI will get better.

Done, pls reapprove the CI workflows.

@hasufell
Copy link
Member

hasufell commented May 2, 2024

Posix standard is very clear: https://pubs.opengroup.org/onlinepubs/009604599/basedefs/sys/time.h.html

time_t         tv_sec      Seconds. 
suseconds_t    tv_usec     Microseconds.

So this doesn't seem theoretical to me at all. It must be fixed.

@liskin
Copy link
Contributor Author

liskin commented May 2, 2024

So this doesn't seem theoretical to me at all. It must be fixed.

Yeah, sure, it's just a bit silly that we have no idea whether there is an actual platform where that code is being used, and if so, what that platform is…

@hasufell
Copy link
Member

hasufell commented May 2, 2024

@hs-viktor any opinions?

@Bodigrim
Copy link
Contributor

@vdukhovni what do you think about this?

@hasufell
Copy link
Member

I'm fairly confident about this.

@liskin ping me in a week (or at Zurihac) and I'll merge, unless there are objections in the meantime.

@liskin
Copy link
Contributor Author

liskin commented Jun 7, 2024

@liskin ping me in a week (or at Zurihac) and I'll merge, unless there are objections in the meantime.

Won't be attending ZuriHac this year unfortunately, so just a ping. 🍻

@Bodigrim Bodigrim merged commit 8183e05 into haskell:master Jun 17, 2024
22 checks passed
@Bodigrim
Copy link
Contributor

Thanks!

@liskin liskin deleted the ctimeval-t64 branch June 18, 2024 09:03
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