date/ls: Switch from chrono to jiff #7894
Conversation
|
GNU testsuite comparison: |
There was a problem hiding this comment.
Pull Request Overview
This PR switches the time handling in both the date and ls utilities from chrono to jiff to improve timezone handling and offer optional timezone database embedding. Key changes include updated tests for expanded timezone coverage (in test_date.rs), a refactoring of ls’s time formatting logic to remove the legacy TimeStyler in favor of jiff’s API (in ls.rs), and corresponding dependency updates in the Cargo.toml files and related build configurations.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/by-util/test_date.rs | Updated testing of timezone handling with various locales and UTC flags |
| src/uu/ls/src/ls.rs | Refactored time formatting code: removed TimeStyler and updated display_date usage |
| src/uu/ls/Cargo.toml | Added jiff dependency with new features |
| src/uu/date/src/date.rs | Migrated date parsing/formatting from chrono to jiff, with updated error handling |
| src/uu/date/Cargo.toml | Added jiff dependency with appropriate feature flags |
| Cross.toml | Added pre-build step to install tzdata |
| Cargo.toml | Updated global dependencies to include jiff’s libraries |
| .github/workflows/CICD.yml | Removed redundant Cross.toml creation step |
Comments suppressed due to low confidence (1)
src/uu/ls/src/ls.rs:2889
- [nitpick] Now that display_date writes directly to the provided buffer and returns a UResult, its naming might be misleading. Consider renaming it (e.g. to write_date) to clarify that it modifies the buffer in place.
output_display.extend(display_date(md, config, state, &mut output_display)?);
| match parse_datetime::parse_datetime(s.as_ref()) { | ||
| Ok(date) => { | ||
| let timestamp = | ||
| Timestamp::new(date.timestamp(), date.timestamp_subsec_nanos() as i32).unwrap(); |
There was a problem hiding this comment.
Using unwrap() here may lead to a panic if Timestamp::new ever fails. Consider handling the potential error explicitly to make the code more robust.
| Timestamp::new(date.timestamp(), date.timestamp_subsec_nanos() as i32).unwrap(); | |
| Timestamp::new(date.timestamp(), date.timestamp_subsec_nanos() as i32)?; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
for once, it might make sense ?! :)
There was a problem hiding this comment.
Ah, yeah, it does make some sort of sense... But:
The suggestion doesn't build:
?couldn't convert the error to(std::string::String, ParseDateTimeError)
The error cannot really happen anyway:
This returns an error if the given components would correspond to an instant outside the supported range. Also, nanosecond is limited to the range -999,999,999..=999,999,999.
So yes, technically I could handle the error and convert it, but since timestamp() and timestamp_subsec_nanos() both come from a good library (jiff), and go into another good library (chrono), I think we can just unwrap.
And we can get rid of this conversion once we switch parse_datetime to jiff (I haven't found time to look into that yet...).
|
Rebased, and also cleaned up I'll try to continue this conversion (maybe will look at parse_datetime next). |
|
GNU testsuite comparison: |
Also adds cargo dependency.
This reverts commit fc6b896. This also reverts the change from new to new_lenient, we'll recover that later as part of the jiff conversion.
From code provided in uutils#7852 by @BurntSushi. Depending on the benchmarks, there is _still_ a small performance difference (~4%) vs main, but it's seen mostly on small trees getting printed repeatedly, which is probably not a terribly interesting use case.
[drinkcat: separated test changes]
Also test %z/%Z formats.
Linux tests require that now, as we now assume /usr/share/zoneinfo is present.
Nobody needs it anymore.
|
Rebased. @sylvestre do you think we could go ahead with this part of the conversion? We get the most bang for the buck I'd like to look into |
|
GNU testsuite comparison: |
Also, the comment does not fully apply anymore, so we can leave it more open-ended to figure out how to support locale.
Using the current time requires a bit of care, but it's nice to have a test that doesn't use a fixed date as input.
|
GNU testsuite comparison: |
|
Good work, thanks! |
|
+1 |
jiffhandling of timezones makes our life so much easier. Also, jiff providesa convenient feature to only embed the timezone database when necessary (e.g. Windows).
TZ#7497, date: Does not correctly useUTCwhenTZis empty #7498, date: Timezone abbreviation conversion should take into account the date to be parsed #7659.(that did not fix all issues, FWIW):
Part of the tests taken from #7597 (@jadijadi FYI), then refactored on top of those to add more test cases.
@BurntSushi thanks for your support!
--
Cross.toml: Install tzdata in container
Linux tests require that now, as we now assume /usr/share/zoneinfo
is present.
test_date: Extend coverage to a lot more timezones
Also test %z/%Z formats.
date: Add more TZ tests
[drinkcat: separated test changes]
ls: switch to lenient formating configuration
ls: Avoid additional String creation/copy in display_date
From code provided in #7852 by @BurntSushi.
Depending on the benchmarks, there is still a small performance
difference (~4%) vs main, but it's seen mostly on small trees
getting printed repeatedly, which is probably not a terribly
interesting use case.
ls: cache recent time threshold in jiff implementation
ls: convert to jiff
Revert "ls: Optimize time formatting"
This reverts commit fc6b896.
This also reverts the change from new to new_lenient, we'll
recover that later as part of the jiff conversion.
date: switch from chrono to jiff
Also adds cargo dependency.