-
Notifications
You must be signed in to change notification settings - Fork 892
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
Replace humantime crate with jiff #7261
base: main
Are you sure you want to change the base?
Conversation
Thank you, as jiff isn't 100% compatible with humantime I think this constitutes a breaking change and will therefore have to wait for the next breaking release in likely ~6 months time... That being said now that an advisory has been published we might want to just push ahead with this to avoid undue angst... I'll let other maintainers weigh in Edit: tbh given how small humantime actually is we probably could just vend the parser locally as a temporary measure... AFAICT there isn't actually an issue with the code |
It depends on what you're doing, but With that said, in this PR, the |
Given these are timeouts I suspect few users are configuring anything larger than an hour. Are there any differences when parsing such? |
Fixes CI failures due to `humantime` being marked as unmaintained. It looks like it will take some time for `object_store` to move on, so I'm adding [`RUSTSEC-2025-0014`](https://rustsec.org/advisories/RUSTSEC-2025-0014) to the ignore list for now: * apache/arrow-rs#7264 * apache/arrow-rs#7261 * Reminder issue: #9255 --------- Co-authored-by: Andreas Reich <[email protected]>
That's a good point and a good question. The major difference is definitely that parsing into let (mut sec, nsec) = match &self.src[start..end] {
"nanos" | "nsec" | "ns" => (0u64, n),
"usec" | "us" => (0u64, n.mul(1000)?),
"millis" | "msec" | "ms" => (0u64, n.mul(1_000_000)?),
"seconds" | "second" | "secs" | "sec" | "s" => (n, 0),
"minutes" | "minute" | "min" | "mins" | "m"
=> (n.mul(60)?, 0),
"hours" | "hour" | "hr" | "hrs" | "h" => (n.mul(3600)?, 0),
"days" | "day" | "d" => (n.mul(86400)?, 0),
"weeks" | "week" | "w" => (n.mul(86400*7)?, 0),
"months" | "month" | "M" => (n.mul(2_630_016)?, 0), // 30.44d
"years" | "year" | "y" => (n.mul(31_557_600)?, 0), // 365.25d
_ => {
return Err(Error::UnknownUnit {
start, end,
unit: self.src[start..end].to_string(),
value: n,
});
}
}; Jiff's "friendly" duration format has a grammar, and indeed, for hour/minute/second/millisecond/microsecond/nanosecond units, Jiff can parse all of the units that The other direction is different, that is, can |
Fixes CI failures due to `humantime` being marked as unmaintained. It looks like it will take some time for `object_store` to move on, so I'm adding [`RUSTSEC-2025-0014`](https://rustsec.org/advisories/RUSTSEC-2025-0014) to the ignore list for now: * apache/arrow-rs#7264 * apache/arrow-rs#7261 * Reminder issue: #9255 --------- Co-authored-by: Andreas Reich <[email protected]>
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.
That being said now that an advisory has been published we might want to just push ahead with this to avoid undue angst... I'll let other maintainers weigh in
I think this is a good change and that the removal of a RUSTSEC is worth any potential issue with parsing questionable durations.
Given @BurntSushi 's long history of maintaining core crates in the Rust ecosystem and the current adoption of jiff I think this is a good choice
BTW if/when this PR is merged, I think we can/should consider backporting this change to the object_store 0.11 line so others can pick up the change before 0.12.0
percolates through the ecosystem
The `humantime` package is no longer maintained. We should move to avoid using it. We were using it indirectly via `env_logger` and `object_store`. This PR updates `env_logger` to the latest version (which uses `jiff` instead of `humantime`). `object_store` is one of our core dependencies and we generally update it as new versions are released. If (and when) `humantime` is replaced with `jiff` we will remove the dependency then. This may be several months. apache/arrow-rs#7261 can be used for tracking. Until then we should suppress the advisory.
|
Rationale for this change
The crate
humantime
appears to be unmaintained. There's open PR in RustSec's advisory-db about this: rustsec/advisory-db#2249The crates
clap
andenv_logger
have already made the switch fromhumantime
tojiff
:Since new version of
env_logger
is already published theobject_store
crate is now second largest user ofhumantime
crate by crates.io downloads counts and the largest user after new version ofclap
is released.What changes are included in this PR?
Replace
humantime
crate withjiff
. The functionality should stay the same.Are there any user-facing changes?
There should be none.
Fixes #7264