-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Use system timezone database when available #7849
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
base: main
Are you sure you want to change the base?
Conversation
iana_time_zone::get_timezone doesn't cover all cases, so we need to handle some of the timezone parsing manually. Also add a bunch of TODO, this function is by no means complete. (change similar to uutils#7597 for this file)
This function will still need some love, but at least with this, we have basic tests.
Unless we're on Windows. Fixes uutils#7504, reduces ls/date executable size by about 2 MB.
Instead of repeating complex logic (and its inverse), set a feature in build.rs.
Linux tests require that now, as we now assume /usr/share/zoneinfo is present.
Not 100% clear why, but setting TZ in CI is not fully reliable, so let's pass a parameter instead.
/// | ||
/// We need this function even for local dates as chrono(_tz) does not provide a | ||
/// way to convert Local to a fully specified timezone with abbreviation | ||
/// (<https://github.com/chronotope/chrono-tz/issues/13>). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should link to chronotope/chrono#960 here, instead.
GNU testsuite comparison:
|
Are you aware of jiff? (I am its author.) It does all of this for you. I did a quick look and I believe it fixes all of your todos as well. |
@BurntSushi of course, who does not know you, your great work and jiff ? :) |
Yeah it just looks like y'all are fighting with Chrono quite a bit. I'm also working on making some extension points to Jiff's |
@BurntSushi thanks, looks like we've been fighting similar problems ,-) I'll try playing with jiff then, will let you know how that goes. |
There are a lot of challenges around timezone handling, and IMHO some refactoring will be needed.
Decided to focus on #7504 for now: we use a system timezone database is available through
tzfile
.Also, added some tests, and a bunch of TODO in the code to give pointers to the next person fixing this (maybe me...).
Shrinks
date
/ls
by about 2 megabytes on Linux:uucore: custom_tz_fmt: test: Setting TZ is not reliable
Not 100% clear why, but setting TZ in CI is not fully reliable,
so let's pass a parameter instead.
Cross.toml: Install tzdata in container
Linux tests require that now, as we now assume /usr/share/zoneinfo
is present.
uucore: custom_tz_fmt: Set an embed_tz feature based on platform
Instead of repeating complex logic (and its inverse), set a
feature in build.rs.
fuzz/Cargo.lock: Update
uucore: Use tzfile instead of chrono-tz
Unless we're on Windows.
Fixes #7504, reduces ls/date executable size by about 2 MB.
uucore: custom_tz_fmt: use format("%Z")
uucore: custom_tz_fmt: Add abbreviation tests
This function will still need some love, but at least with this,
we have basic tests.
uucore: custom_tz_fmt: Improve TZ environment variable handling
iana_time_zone::get_timezone doesn't cover all cases, so we need
to handle some of the timezone parsing manually.
Also add a bunch of TODO, this function is by no means complete.
(change similar to #7597 for this file)