Skip to content
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

Switch time to jiff for time formatting in ICE dumps #139315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clubby789
Copy link
Contributor

@clubby789 clubby789 commented Apr 3, 2025

Due to jhpratt/deranged#21, Clippy, R-A and Miri currently fail to build if we bump to 0.4.1, pulled in via time. Add some specific type annotations so we don't have to just pin it.

I can open 3 PRs to the tool repos if preferred, but I thought it might be easier to do this than to pin the transitive dep and go back and remove it once the changes are synced back.

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

It's fine from the r-a side.

@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2025

Maybe we should re-consider our use of this dependency if it breaks entirely unrelated code in this way.

@rust-log-analyzer

This comment has been minimized.

@clubby789
Copy link
Contributor Author

Maybe we should re-consider our use of this dependency if it breaks entirely unrelated code in this way.

Currently, it's pulled in via time, which is only used in rustc_driver_impl for formatting the rustc-ice files, which is probably avoidable. Will look into that

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 3, 2025
@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@clubby789
Copy link
Contributor Author

I don't feel very confident about having our own bespoke formatting, so tried switching over to chrono, which does have the downside of pulling in a different set of dependencies - which at least currently don't cause issues all over our tools and tests

@clubby789 clubby789 changed the title Bump deranged and fix breakage Switch time to chrono for time formatting in ICE dumps Apr 3, 2025
@ChrisDenton
Copy link
Member

It seems like a heavy weight dependency just for formatting rustc-ice files. Is there nothing else that can be used?

@Veykril
Copy link
Member

Veykril commented Apr 3, 2025

https://docs.rs/jiff/latest/jiff/ maybe?

@clubby789 clubby789 changed the title Switch time to chrono for time formatting in ICE dumps Switch time to jiff for time formatting in ICE dumps Apr 3, 2025
@clubby789
Copy link
Contributor Author

clubby789 commented Apr 3, 2025

Certainly seems more lightweight (and is already in use in the tree)

@jhpratt
Copy link
Member

jhpratt commented Apr 3, 2025

FYI I am reconsidering whether to yank the release in question given the apparently larger scope of breakage than I initially assumed would happen. Keep an eye on jhpratt/deranged#21 for updates. I'll have a definitive answer in a day or two max (currently quite busy and am abroad).

@jhpratt
Copy link
Member

jhpratt commented Apr 5, 2025

re. above, the relevant version was yanked, so version bumps for Rust should work as expected now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.