Skip to content

Conversation

@ammaraskar
Copy link
Member

@ammaraskar ammaraskar commented Mar 10, 2025

DO NOT MERGE.

This is a work-in-progress/proof-of-concept pull request for not using localtime on Windows for negative epoch values but instead using win32 APIs. We basically have a set of long-standing issues and work-arounds/platform specific code for the fact that localtime on Windows does not support negative numbers, unlike our other platforms.

Some examples from a quick search include:

The changes to Python/pytime.c are the ones to look at, the rest are just enabling tests/removing work-arounds. Note that I had to use an if-guard to only use the new win32 apis when the unix timestamp is >= 0 because as it turns out, mktime on Windows has this behavior where it just doesn't account for DST correctly. Without it, tests that rely on round-tripping mktime and localtime fail. (See this repo for more details on that)

There is also the question on how to support tm_yday and tm_isdst. I have an implementation of tm_yday and a commented out one for tm_isdst.

/cc @zooba for thoughts on this approach, whether it's worth pursuing.
/cc @eryksun for Windows expertise on the APIs/methodology

@zooba
Copy link
Member

zooba commented Mar 10, 2025

On the overall approach, I guess it fills in a gap, but I'd prefer to have a single codepath that handles it all. Dates/times are hard though, and I'm not even going to pretend I know how this function is actually meant to behave.

My preference would be to use the best available OS API for the entire job, rather than splitting it up. If no such API exists, something like this I'd think we could implement ourselves and stop relying on the OS entirely.

But if the people who understand this particular subject better than me say it's not safe to make a change like that, then I'm happy enough to go with their recommendation. I'm not sure who those people are right now.

@ammaraskar
Copy link
Member Author

My preference would be to use the best available OS API for the entire job, rather than splitting it up

Switching mktime to also use the symmetrical win32 APIs would be able to achieve that since it would avoid that mktime bug I mentioned. I'll try updating this PR to do that, avoiding the split logic based on the t value and see if it works better/how much complexity it involves.

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