Skip to content

fix(index): Apply feedback from Cargo team #16369

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

Jump to bottom
Merged
weihanglo merged 7 commits into from
Dec 10, 2025
Merged

fix(index): Apply feedback from Cargo team #16369

weihanglo merged 7 commits into from
Dec 10, 2025
+165 −17

Conversation

@epage
Copy link
Contributor

@epage epage commented Dec 9, 2025

What does this PR try to resolve?

Questions from #16270

  • How should we deal with Summary pubtime parse errors?
    • Treat the entry as invalid, rather than recovering since we have no reason to allow other layouts in the future
  • How strict should we be on the Summary pubtime format?
    • Limit to what we specify which is pretty strict
  • What precision do we want?
    • No more than seconds seems the right choice.

Through the conversation, it also came up that we should be very explicit that pubtime is only for publish time and not updates like yanked

How to test and review this PR?

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-registries Area: registries labels Dec 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added Command-generate-lockfile S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2025
@epage epage mentioned this pull request Dec 9, 2025
Open
3 tasks
@epage
Copy link
Contributor Author

epage commented Dec 9, 2025 

 ---- cache_lock::download_then_mutate stdout ----

thread 'cache_lock::download_then_mutate' (10088) panicked at tests\testsuite\cache_lock.rs:15:5:
test did not finish within 10 attempts (1.0029456s total)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    cache_lock::download_then_mutate

@weihanglo
Copy link
Member

weihanglo commented Dec 9, 2025 •
edited
Loading

cache_lock tests seem becoming flaky recently on Windows MSVC (also happened in #16364 (comment))

0xPoe
0xPoe reviewed Dec 9, 2025
View reviewed changes
Copy link
Member

@0xPoe 0xPoe left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Thanks!

View changes since this review

0xPoe
0xPoe approved these changes Dec 9, 2025
View reviewed changes
Copy link
Member

@0xPoe 0xPoe left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Thanks! :shipit:

View changes since this review

@0xPoe 0xPoe added this pull request to the merge queue Dec 9, 2025
weihanglo
weihanglo reviewed Dec 9, 2025
View reviewed changes
crates/cargo-util-schemas/src/index.rs
/// In ISO8601 with UTC timezone (e.g. 2025-11-12T19:30:12Z)
pub pubtime: Option<String>,
#[cfg_attr(feature = "unstable-schema", schemars(with = "Option<String>"))]
pub pubtime: Option<jiff::Timestamp>,
Copy link
Member

@weihanglo weihanglo Dec 9, 2025

Choose a reason for hiding this comment

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

Do we intend to expose the concrete jiff::Timestamp to users? Should we wrap it with our own type?

(Similar discussion in #15293 (comment). I am fine with either)

Copy link
Contributor Author

@epage epage Dec 10, 2025

Choose a reason for hiding this comment

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

My comment from that thread

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.

Just adding a field is a breaking change so not to worried and adding a custom type seems like boiler plate with little benefit

Copy link
Member

@weihanglo weihanglo Dec 10, 2025

Choose a reason for hiding this comment

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

Thanks. Just want to make sure this is intentional. Also cargo-util-schemas is a package we are not worried about bumping version.

@0xPoe 0xPoe removed this pull request from the merge queue due to a manual request Dec 9, 2025
weihanglo
weihanglo reviewed Dec 9, 2025
View reviewed changes
crates/cargo-util-schemas/src/index.rs

pub fn parse_pubtime(s: &str) -> Result<jiff::Timestamp, jiff::Error> {
let dt = jiff::civil::DateTime::strptime("%Y-%m-%dT%H:%M:%SZ", s)?;
if s.len() == 20 {
Copy link
Member

@weihanglo weihanglo Dec 9, 2025

Choose a reason for hiding this comment

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

Cargo isn't future-proof for years beyond 9999 😆

Copy link
Member

@0xPoe 0xPoe Dec 9, 2025

Choose a reason for hiding this comment

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

I hope it can last that long! Perhaps this code still is buried in the Arctic mountain at that time🤣

@weihanglo weihanglo added this pull request to the merge queue Dec 10, 2025
Merged via the queue into rust-lang:master with commit 334b7b6 Dec 10, 2025
53 of 58 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2025
@epage epage deleted the pubtime branch December 10, 2025 01:57
@weihanglo weihanglo mentioned this pull request Dec 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xPoe 0xPoe 0xPoe approved these changes

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels

A-cli Area: Command-line interface, option parsing, etc. A-registries Area: registries Command-generate-lockfile

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@epage @rustbot @weihanglo @0xPoe