Skip to content

Replace time crate with jiff #15293

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.

Sign up for GitHub

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

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

Replace time crate with jiff #15293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5937d1
Replace time crate with jiff
oherrala Mar 10, 2025
22adbe2
chore: bump cargo-credential
oherrala Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ ignore = "0.4.23"
im-rc = "15.1.0"
indexmap = "2.7.1"
itertools = "0.14.0"
jiff = { version = "0.2.3", default-features = false, features = [ "std" ] }
jiff = { version = "0.2.3", default-features = false, features = [ "std", "serde" ] }
jobserver = "0.1.32"
lazycell = "1.3.0"
libc = "0.2.169"
Expand Down Expand Up @@ -102,7 +102,6 @@ snapbox = { version = "0.6.21", features = ["diff", "dir", "term-svg", "regex",
tar = { version = "0.4.43", default-features = false }
tempfile = "3.16.0"
thiserror = "2.0.11"
time = { version = "0.3.37", features = ["parsing", "formatting", "serde"] }
toml = "0.8.20"
toml_edit = { version = "0.22.23", features = ["serde"] }
tracing = { version = "0.1.41", default-features = false, features = ["std"] } # be compatible with rustc_log: https://github.com./rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9
Expand Down Expand Up @@ -208,7 +207,6 @@ supports-unicode.workspace = true
tar.workspace = true
tempfile.workspace = true
thiserror.workspace = true
time.workspace = true
toml.workspace = true
toml_edit.workspace = true
tracing = { workspace = true, features = ["attributes"] }
Expand Down
2 changes: 1 addition & 1 deletion crates/cargo-test-support/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ flate2.workspace = true
git2.workspace = true
glob.workspace = true
itertools.workspace = true
jiff.workspace = true
pasetors.workspace = true
regex.workspace = true
serde = { workspace = true, features = ["derive"] }
serde_json.workspace = true
snapbox.workspace = true
tar.workspace = true
time.workspace = true
toml.workspace = true
url.workspace = true
walkdir.workspace = true
Expand Down
10 changes: 5 additions & 5 deletions crates/cargo-test-support/src/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use cargo_util::paths::append;
use cargo_util::Sha256;
use flate2::write::GzEncoder;
use flate2::Compression;
use jiff::{SignedDuration, Timestamp};
use pasetors::keys::{AsymmetricPublicKey, AsymmetricSecretKey};
use pasetors::paserk::FormatAsPaserk;
use pasetors::token::UntrustedToken;
Expand All @@ -58,10 +59,9 @@ use std::fs::{self, File};
use std::io::{BufRead, BufReader, Read, Write};
use std::net::{SocketAddr, TcpListener, TcpStream};
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::thread::{self, JoinHandle};
use tar::{Builder, Header};
use time::format_description::well_known::Rfc3339;
use time::{Duration, OffsetDateTime};
use url::Url;

/// Path to the local index for psuedo-crates.io.
Expand Down Expand Up @@ -910,9 +910,9 @@ impl HttpServer {
v: Option<u8>,
}
let message: Message<'_> = t!(serde_json::from_str(trusted_token.payload()).ok());
let token_time = t!(OffsetDateTime::parse(message.iat, &Rfc3339).ok());
let now = OffsetDateTime::now_utc();
if (now - token_time) > Duration::MINUTE {
let token_time = t!(Timestamp::from_str(message.iat).ok());
let now = Timestamp::now();
if now.duration_since(token_time) > SignedDuration::from_mins(1) {
return false;
}
if private_key_subject.as_deref() != message.sub {
Expand Down
4 changes: 2 additions & 2 deletions credential/cargo-credential/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "cargo-credential"
version = "0.4.8"
version = "0.4.9"
rust-version.workspace = true
edition.workspace = true
license.workspace = true
Expand All @@ -10,10 +10,10 @@ description = "A library to assist writing Cargo credential helpers."

[dependencies]
anyhow.workspace = true
jiff.workspace = true
serde = { workspace = true, features = ["derive"] }
serde_json.workspace = true
thiserror.workspace = true
time.workspace = true

[target.'cfg(unix)'.dependencies]
libc.workspace = true
Expand Down
8 changes: 4 additions & 4 deletions credential/cargo-credential/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@
#![allow(clippy::print_stderr)]
#![allow(clippy::print_stdout)]

use jiff::Timestamp;
use serde::{Deserialize, Serialize};
use std::{fmt::Display, io};
use time::OffsetDateTime;

mod error;
mod secret;
Expand Down Expand Up @@ -202,8 +202,8 @@ pub enum CacheControl {
Never,
/// Cache this result and use it for subsequent requests in the current Cargo invocation until the specified time.
Expires {
#[serde(with = "time::serde::timestamp")]
expiration: OffsetDateTime,
#[serde(with = "jiff::fmt::serde::timestamp::second::required")]
expiration: Timestamp,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a public API change and needs a major version bump?

Wonder why cargo-semver-checks didn't report to us.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, should we wait for jiff 1.0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or change it to other integer type or wrapper type, so it is completely opaque.

There is no guarantee that jiff@2 won't come out a week after jiff@1 is out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree an integer or wrapper type makes sense for something like this.

There is no guarantee that jiff@2 won't come out a week after jiff@1 is out.

Just to comment on this, it is my intent that jiff will stay at 1.0 indefinitely. For a jiff 2.0 to happen a week or even 1 month after jiff 1.0 is released, I think something spectacular would have had to go wrong.

Otherwise, I do have a track record of sticking to 1.0. regex is still at 1.0 even though there are mistakes in its API I would like to fix. Same with bstr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @BurntSushi! I totally agree with it and fully trust you as a maintainer of those awesome crates. I meant to express that regardless it is 0.2 or 1.0, a major version bump is a major version bump. If we have fear of re-export we should make it opaque.

Sorry I didn't mean to make you feel bad 😞.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I didn't feel bad, I just want to make sure the right kinds of expectations are telegraphed. :) I just mean it would be a spectacular failure on my part if 2.0 were released right after 1.0.

My plan is to release Jiff 1.0 this summer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and we already had this problem with time in the puiblic API. I was just wondering if we should consolidate the breaking changes or going ahead and moving forward. No strong opinion. So long as the serialization format is unchanged, breaking changes to this API are not too big of a deal; the target audience is very small.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the braking changes (whatever they are) be done in another PR before this? Or in this PR?

Copy link
Member

@weihanglo weihanglo Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are removing time in this PR, the breaking change seems better to be in the same PR. Could be one more commit if needed.

Slightly prefer the approach making it opaque. Not really strong though.

},
/// Cache this result and use it for all subsequent requests in the current Cargo invocation.
Session,
Expand Down Expand Up @@ -320,7 +320,7 @@ mod tests {
#[test]
fn cache_control() {
let cc = CacheControl::Expires {
expiration: OffsetDateTime::from_unix_timestamp(1693928537).unwrap(),
expiration: Timestamp::from_second(1693928537).unwrap(),
};
let json = serde_json::to_string(&cc).unwrap();
assert_eq!(json, r#"{"cache":"expires","expiration":1693928537}"#);
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/util/auth/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ use cargo_credential::{
};

use core::fmt;
use jiff::Timestamp;
use serde::Deserialize;
use std::error::Error;
use time::{Duration, OffsetDateTime};
use std::time::Duration;
use url::Url;

use crate::core::SourceId;
Expand Down Expand Up @@ -619,7 +620,7 @@ fn auth_token_optional(
if let Some(cached_token) = cache.get(url) {
if cached_token
.expiration
.map(|exp| OffsetDateTime::now_utc() + Duration::minutes(1) < exp)
.map(|exp| Timestamp::now() + Duration::from_secs(60) < exp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing, but you could do Timestamp::now() + SignedDuration::from_mins(1) here.

Copy link
Contributor Author

@oherrala oherrala Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some benefit to use SignedDuration instead of std's Duration? Latter is just shorter to write and does the same thing :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You get the from_mins constructor. But otherwise, in this case, no.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially sicne you use the above suggestion in your changes to crates/cargo-test-support/src/registry.rs

.unwrap_or(true)
{
if cached_token.operation_independent || matches!(operation, Operation::Read) {
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/util/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ use cargo_credential::Secret;
use cargo_util::paths;
use cargo_util_schemas::manifest::RegistryName;
use curl::easy::Easy;
use jiff::Timestamp;
use lazycell::LazyCell;
use serde::de::IntoDeserializer as _;
use serde::Deserialize;
use serde_untagged::UntaggedEnumVisitor;
use time::OffsetDateTime;
use toml_edit::Item;
use url::Url;

Expand Down Expand Up @@ -155,7 +155,7 @@ enum WhyLoad {
#[derive(Debug)]
pub struct CredentialCacheValue {
pub token_value: Secret<String>,
pub expiration: Option<OffsetDateTime>,
pub expiration: Option<Timestamp>,
pub operation_independent: bool,
}

Expand Down
5 changes: 2 additions & 3 deletions src/cargo/util/credential/paseto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use pasetors::{
keys::{AsymmetricKeyPair, AsymmetricPublicKey, AsymmetricSecretKey, Generate},
paserk::FormatAsPaserk,
};
use time::{format_description::well_known::Rfc3339, OffsetDateTime};
use url::Url;

use crate::{
Expand Down Expand Up @@ -103,10 +102,10 @@ impl<'a> Credential for PasetoCredential<'a> {
.expose();
let kip: pasetors::paserk::Id = (&public).into();

let iat = OffsetDateTime::now_utc();
let iat = jiff::Timestamp::now();

let message = Message {
iat: &iat.format(&Rfc3339).unwrap(),
iat: &iat.to_string(),
sub: secret_key_subject.as_deref(),
mutation: match operation {
Operation::Publish { .. } => Some("publish"),
Expand Down