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

Consolidating dependencies #6034

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

michaelciraci
Copy link
Contributor

Solve #6033

r? @ytmimi

@michaelciraci michaelciraci marked this pull request as draft January 21, 2024 16:52
@michaelciraci michaelciraci marked this pull request as ready for review January 21, 2024 18:07
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

We link to dirs in Configurations.md. Can you also update the link? I haven't had a chance to review these dependency updates yet, but I'll follow up once I do.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 25, 2024

The dirs bumpt to 5.0.1 looks good. There don't seem to be any updates that would break us as no changes were made to the dirs::home_dir() and dirs::config_dir() functions. It does look like there's new support for non-roaming configuration on Windows that we might consider adding, but that shouldn't impact the dependency bump.

@michaelciraci any chance you know where I can find out what changed with cargo_metadata between 0.15.4 and 0.18.0? The CHANGELOG isn't much help

@michaelciraci
Copy link
Contributor Author

Hmm, yeah you're right. I started going through it, but there's quite a few changes. Is there something in particular you're looking for?

@ytmimi
Copy link
Contributor

ytmimi commented Jan 26, 2024

Any changes to how cargo_metadata::MetadataCommand works?

We use it in cargo-fmt.

fn get_cargo_metadata(manifest_path: Option<&Path>) -> Result<cargo_metadata::Metadata, io::Error> {
let mut cmd = cargo_metadata::MetadataCommand::new();
cmd.no_deps();
if let Some(manifest_path) = manifest_path {
cmd.manifest_path(manifest_path);
}
cmd.other_options(vec![String::from("--offline")]);
match cmd.exec() {
Ok(metadata) => Ok(metadata),
Err(_) => {
cmd.other_options(vec![]);
match cmd.exec() {
Ok(metadata) => Ok(metadata),
Err(error) => Err(io::Error::new(io::ErrorKind::Other, error.to_string())),
}
}
}
}
If you could help look into it that would give me more confidence in making the change. Assuming that nothing material has changed then we should be good to go

@michaelciraci
Copy link
Contributor Author

We should be fine to upgrade. There are several internal changes (such as using thiserror instead of a custom error type), more use of enums over Strings, and some additional data fields. But the main api rustfmt uses is the same from 0.15.4 -> 0.18.

The complete diff is here if you'd like to dig into the details yourself: oli-obk/cargo_metadata@0.15.0...0.18.1

@ytmimi ytmimi force-pushed the update-dirs-cargo-metadata branch from c93f90d to 391bb22 Compare January 29, 2024 04:55
@ytmimi
Copy link
Contributor

ytmimi commented Jan 29, 2024

@michaelciraci thank you very much for taking the time to review those changes. It was a huge help not to have to go through it all myself 🙏🏼. This is next on my list to merge (just waiting on CI).

@ytmimi ytmimi merged commit ead0fc9 into rust-lang:master Jan 29, 2024
27 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Jan 29, 2024
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants