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: populate timezone data when formatting time #3203

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marcin-serwin
Copy link

@marcin-serwin marcin-serwin commented Nov 14, 2024

The jv2tm function was zeroing fields of struct tm that were not specified by the standard. However, depending on the libc this produced incorrect timezone data when used together with formatting functions. This change tries to fill the timezone data using either mktime, timegm, or manually.

fixes #2475; fixes #2429.

Example:

TZ=Asia/Tokyo jq -rn '1731627341 | strflocaltime("%F %T %z %Z"), strftime("%F %T %z %Z")'

Before:

2024-11-15 08:35:41 +0000 JST
2024-11-14 23:35:41 +0000 JST

After:

2024-11-15 08:35:41 +0900 JST
2024-11-14 23:35:41 +0000 GMT

@itchyny
Copy link
Contributor

itchyny commented Jan 20, 2025

I'm afraid the timezone offset is not fixed even with this patch.

# 1.7.1
 $ TZ=Asia/Tokyo jq -rn '1731627341 | strflocaltime("%F %T %z %Z"), strftime("%F %T %z %Z")'  
2024-11-15 08:35:41 +0900 JST
2024-11-14 23:35:41 +0900 JST

# marcin-serwin:strftime-fix
 $ TZ=Asia/Tokyo ./jq -rn '1731627341 | strflocaltime("%F %T %z %Z"), strftime("%F %T %z %Z")'
2024-11-15 08:35:41 +0900 JST
2024-11-14 23:35:41 +0900 UTC

# 1.7.1
 $ TZ=Europe/Paris jq -rn '1731627341 | strflocaltime("%F %T %z %Z"), strftime("%F %T %z %Z")'
2024-11-15 00:35:41 +0100 CET
2024-11-14 23:35:41 +0100 CET

# marcin-serwin:strftime-fix
 $ TZ=Europe/Paris ./jq -rn '1731627341 | strflocaltime("%F %T %z %Z"), strftime("%F %T %z %Z")'
2024-11-15 00:35:41 +0100 CET
2024-11-14 23:35:41 +0100 UTC

 $ uname -v
Darwin Kernel Version 23.4.0: Fri Mar 15 00:12:41 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T8103

The `jv2tm` function was zeroing fields of `struct tm` that were not
specified by the standard. However, depending on the libc this produced
incorrect timezone data when used together with formatting functions.
This change tries to fill the timezone data using either `mktime`,
`timegm`, or manually.
@marcin-serwin
Copy link
Author

This issue is caused by a bug in the Apple's Libc implementation. Specifically in this line the __DARWIN_UNIX03 macro is negated which means that the strftime is ignoring the time offset data present in the tm struct in favor of the older heuristic needed by legacy standards. Looking at other uses of this macro, it seems to enable non-legacy behavior so the negation here is likely a bug since this means that the presence of this macro opts into the legacy behavior. The FreeBSD libc, from which the Apple's one is forked, does not have this issue.

I've added a workaround in the second commit that temporarily sets the global timezone to UTC if it is compiled for apple platforms.

@wader
Copy link
Member

wader commented Jan 26, 2025

Nice digging 👍 jq can be used as a library does that make it a bad idea to use setenv? 🤔

Possible to add some tests for this?

Apple's Libc implementation contains a bug which causees it to ignore
the offset data present in the tm struct in favor of the older heuristic
needed by legacy standards. This workaround temporarily sets the global
timezone so it gets picked up during formatting.
@marcin-serwin
Copy link
Author

jq can be used as a library does that make it a bad idea to use setenv? 🤔

setenv is already used in the my_mktime function for a similar purpose. It can cause issues in multi-threaded programs but I don't know how to avoid that.

Possible to add some tests for this?

I've added some tests in tests/shtest, is this the right place?

@marcin-serwin
Copy link
Author

marcin-serwin commented Jan 27, 2025

I've disabled the tests on windows. The ucrt on Windows always uses the global settings for determining the timezone, which is documented in a note near the end of the strftime docs. I tried to make them work similarly to Apple workaround by using setenv and _tzset but I couldn't make it work and I gave up trying to figure out why. Given that it's a documented behavior for Windows and the jq docs warn that strftime output depends on the operating system I think it's ok to leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strftime uses wrong time zone for %Z strflocaltime always outputs +0000 for %Z
3 participants