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

datetime.fromtimestamp(t).astimezone() fails for 0<=t<86400 in Windows #107078

Open
LefterisJP opened this issue Jul 22, 2023 · 4 comments
Open
Labels
OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@LefterisJP
Copy link

LefterisJP commented Jul 22, 2023

Bug report

dt.datetime.fromtimestamp(t).astimezone() fails for 0<=t<86400 in Windows for at least python 3.9, 3.10, 3.11.
Works fine in Linux, Mac.

>> import datetime as dt
>> dt.datetime.fromtimestamp(0)
datetime.datetime(1970, 1, 1, 1, 0)

>> dt.datetime.fromtimestamp(86400)
datetime.datetime(1970, 1, 2, 1, 0)

>> dt.datetime.fromtimestamp(0).astimezone()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 22] Invalid argument

>> dt.datetime.fromtimestamp(86399).astimezone()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 22] Invalid argument

>> dt.datetime.fromtimestamp(86400).astimezone()
datetime.datetime(1970, 1, 2, 1, 0, tzinfo=datetime.timezone(datetime.timedelta(seconds=3600), 'W. Europe Standard Time'))

A screenshot also:
F1qM1ZmXgAAVrTw

Your environment

  • CPython versions tested on: 3.9, 3.10, 3.11
  • Operating system and architecture: Windows

Additional information

This looks very very related to this issue: #73283 but now it seems to happen for astimezone.

Linked PRs

@LefterisJP LefterisJP added the type-bug An unexpected behavior, bug, or error label Jul 22, 2023
@littlebutt
Copy link
Contributor

I think the bug was introduced by _datetimemodule.c here.

lt = local(u2);

If the t is not large enough, the local method will return -1 since it will minus an epoch time(62135683200).

littlebutt added a commit to littlebutt/cpython that referenced this issue Jul 24, 2023
littlebutt added a commit to littlebutt/cpython that referenced this issue Jul 24, 2023
@littlebutt
Copy link
Contributor

The thing is that if the t you passed is so small, the internal parameter u2 will be negative. Since the method localtime_s cannot accept a negative parameter, it will throw an OSError. I just plused a max_fold_seconds to make sure the u2 is positive. I admit it is not an elegant solotion.

littlebutt added a commit to littlebutt/cpython that referenced this issue Jul 24, 2023
@iritkatriel iritkatriel added OS-windows stdlib Python modules in the Lib dir labels Nov 26, 2023
OjusWiZard added a commit to OjusWiZard/rotki that referenced this issue Jan 23, 2024
Signed-off-by: OjusWiZard <ojuswimail@gmail.com>
OjusWiZard added a commit to OjusWiZard/rotki that referenced this issue Jan 23, 2024
Signed-off-by: OjusWiZard <ojuswimail@gmail.com>
OjusWiZard added a commit to OjusWiZard/rotki that referenced this issue Jan 24, 2024
Signed-off-by: OjusWiZard <ojuswimail@gmail.com>
OjusWiZard added a commit to OjusWiZard/rotki that referenced this issue Jan 24, 2024
Signed-off-by: OjusWiZard <ojuswimail@gmail.com>
LefterisJP pushed a commit to rotki/rotki that referenced this issue Jan 24, 2024
Signed-off-by: OjusWiZard <ojuswimail@gmail.com>
@serhiy-storchaka
Copy link
Member

On Linux timestamp can be negative. Why the interval 0-86400 is so important that it is worth to support it on Windows, but not negative timestamps? It is just a limitation of the platform.

@zooba
Copy link
Member

zooba commented Feb 1, 2024

I suspect the value 0 is important, as it's an obvious edge case (or null case). Investigating that revealed that the first value that works is 86400, and so that's the range.

I guess people are less surprised that -1 doesn't work? TBH, I'd be okay with making this calculation fully Python specific, rather than trying to use OS APIs, assuming we can find a migration path. The CRT emulation clearly isn't compatible enough, and the OS APIs are so much richer that they aren't really compatible either.

There was some discussion about adopting ICU for this kind of stuff... maybe if we ever do that we can have consistent timezone handling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

5 participants