fix: handling of --all when local dep name and dir name differ #3664
Conversation
| .find(|p| p.name == dependency.name); | ||
| let manifest_path = if dependency_package.is_some() { | ||
| PathBuf::from(&dependency_package.unwrap().manifest_path) | ||
| } else { |
There was a problem hiding this comment.
I'm not sure if there's a circumstance where the dependency name wouldn't be found/when the if block woudln't be executed, but have this else block to fallback to the previous behavior just in case.
| @@ -282,7 +282,8 @@ fn get_targets_recursive( | |||
| mut targets: &mut BTreeSet<Target>, | |||
| visited: &mut BTreeSet<String>, | |||
| ) -> Result<(), io::Error> { | |||
| let metadata = get_cargo_metadata(manifest_path)?; | |||
| let metadata = get_cargo_metadata(manifest_path, false)?; | |||
| let metadata_with_deps = get_cargo_metadata(manifest_path, true)?; | |||
There was a problem hiding this comment.
The dependency struct returned from the cargo_metadata crate unsurprisingly does not include info about the manifest location of the dependency.
As such, I figured I could make another call to cargo_metadata to get the metadata including dependencies since that cargo metadata will include the correct manifest path for the dependency, regardless of what package name/directory name are used. That "correct" manifest path is then used below.
There was a problem hiding this comment.
Calling cargo_metadata twice is a bit unfortunate, but I think this is the best we can do right now. A possible approach in the future would be to refactor cargo_metadata so that the cargo_metadata::Dependency contains the manifest location if the dependency is local
There was a problem hiding this comment.
Completely agree 👍
calebcartwright
changed the title
| @@ -282,7 +282,8 @@ fn get_targets_recursive( | |||
| mut targets: &mut BTreeSet<Target>, | |||
| visited: &mut BTreeSet<String>, | |||
| ) -> Result<(), io::Error> { | |||
| let metadata = get_cargo_metadata(manifest_path)?; | |||
| let metadata = get_cargo_metadata(manifest_path, false)?; | |||
| let metadata_with_deps = get_cargo_metadata(manifest_path, true)?; | |||
There was a problem hiding this comment.
Calling cargo_metadata twice is a bit unfortunate, but I think this is the best we can do right now. A possible approach in the future would be to refactor cargo_metadata so that the cargo_metadata::Dependency contains the manifest location if the dependency is local
Refs #3643 (the
cargo fmt --allportion)Fixes issue running
cargo fmt --allwhen local dependencies reside in a directory name that differs from the package name.I created a sample repo here with local dependencies that reside in directories named differently than their package names that can be used to test against. Run
cargo fmt --all -- --checkin the root of the project, and note the formatting errors coming from the local dependency directories.