date/ls: Switch from chrono to jiff #7894
Conversation
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.
|
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...).
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.