fix(path): clarify error message when path dependency has wrong package#16927
fix(path): clarify error message when path dependency has wrong package#16927raushan728 wants to merge 2 commits intorust-lang:masterfrom
Conversation
|
r? @epage rustbot has assigned @epage. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| pub fn load(&self) -> CargoResult<()> { | ||
| let mut package = self.package.borrow_mut(); | ||
| if package.is_none() { | ||
| if !self.path.join("Cargo.toml").exists() { |
There was a problem hiding this comment.
It was an earlier attempt to handle missing Cargo.toml - reverted because it was too broad and broke existing tests. Now i use typed error detection in registry.rs
There was a problem hiding this comment.
From #16927 (comment)
Note for reviewers: The change in
registry.rsis needed to handle case 2 and case 3 (wherebar/exists as a directory but has noCargo.toml). Without it the IO error fires before the resolver reachesactivation_error_path_hints. The suppression is guarded by three conditions: path source, typedManifestErrorwithNotFound, andhas_nearby_manifestsconfirming subdirectory packages exist.
| if s.is_empty() { | ||
| s.push('.'); | ||
| } | ||
| if !s.ends_with('/') { |
There was a problem hiding this comment.
use file_name() to show the actual directory name (e.g. foo/) instead of ./ when the path equals the parent root
There was a problem hiding this comment.
That isn't a complete sentence, making it harder to follow what you mean. I don't see how a file_name() (which?) even fits into this.
There was a problem hiding this comment.
When path == parent_root, strip_prefix returns an empty string. In that case we fall back to path.file_name() to show the directory name (e.g. foo/) rather than an empty or confusing path
There was a problem hiding this comment.
file_name() doesn't make sense here. We should probably be using pathdiff or using an absolute path
The original point I raised was actually about always appending / since that isn't something users are likely to do
|
Note for reviewers: The change in |
| let res = source.query(dep, kind, f).await; | ||
| if let Err(e) = &res { | ||
| if dep.source_id().is_path() | ||
| && crate::core::resolver::errors::is_manifest_not_found(e) | ||
| && has_nearby_manifests(dep) | ||
| { | ||
| return Ok(()); | ||
| } | ||
| } | ||
| res.with_context(|| format!("unable to update {}", source.source_id())) |
There was a problem hiding this comment.
This has really ballooned in complexity, including the idea of "catching" specific errors.
There was a problem hiding this comment.
The registry.rs change was needed because without it, the IO error fires before the resolver reaches the diagnostic code for cases 2/3.
Is there a cleaner way you'd suggest to intercept this early failure so we can still provide helpful hints for those cases? Happy to rework the approach if you have a better path in mind.
There was a problem hiding this comment.
From #16927 (comment)
Regarding
registry.rs- every alternative approach I tried but they broke existing tests. This is the cleanest I could find while keeping everything green.
There was a problem hiding this comment.
I have strong reservations about this approach and would need to understand why there is no other way, rather than being told that.
d4eb6f0 to
d5f8d89
Compare
…ge (rust-lang#15296) When a path dependency points to a directory with a different package, show which package exists there and where the correct one can be found.
This comment was marked as duplicate.
This comment was marked as duplicate.
| let _ = writeln!( | ||
| &mut msg, | ||
| "no matching package named `{}` found at `{}`", | ||
| dep.package_name(), | ||
| rel_path(&path), | ||
| ); | ||
| let _ = write!( | ||
| &mut msg, | ||
| "required by {}", | ||
| describe_path_in_context(resolver_ctx, &parent.package_id()), | ||
| ); |
There was a problem hiding this comment.
Why does is_handled_custom_path_error till exist?
On the surface, it looks like we use it to move the location searched: message into the line before it but it isn't clear why we feel that complexity is worth it.
| /// Checks if an error is a "manifest not found" error (Cargo.toml missing). | ||
| /// This is used to allow the resolver to continue and provide helpful hints | ||
| /// for path dependencies instead of failing immediately with a load error. | ||
| pub fn is_manifest_not_found(err: &anyhow::Error) -> bool { |
There was a problem hiding this comment.
If we keep this, why is it here when only used in the Source?
View all comments
Fixes #15296
When a path dependency specifies a package name that doesn't match what's
found at that path, Cargo now shows helpful hints about what packages exist
nearby instead of the confusing "location searched" message.