fix: only examine base URL when matching package #13110
Conversation
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
|
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
epage
changed the title
| let package_base_url = format!( | ||
| "{}://{}", | ||
| u.scheme(), | ||
| u.host_str().expect("package spec url should have a host") | ||
| ); | ||
| let source_id = package_id.source_id(); | ||
| let source_id_url = source_id.url(); | ||
| let source_id_base_url = format!( | ||
| "{}://{}", | ||
| source_id_url.scheme(), | ||
| source_id_url | ||
| .host_str() | ||
| .expect("source id url should have a host") | ||
| ); | ||
| // Examine only the base URL, as the package spec URL might include a package name within its path. | ||
| if package_base_url != source_id_base_url { |
There was a problem hiding this comment.
I don't think its correct to completely ignore paths within the URLs, like with git sources.
There was a problem hiding this comment.
Yeah. I found it from the failed CI. We also support file URLs.
| assert!(PackageIdSpec::parse("https://example.com/foo") | ||
| .unwrap() | ||
| .matches(foo)); | ||
| assert!(!PackageIdSpec::parse("https://example.com/foo#1.2.3") | ||
| assert!(PackageIdSpec::parse("https://example.com/foo#1.2.3") | ||
| .unwrap() |
There was a problem hiding this comment.
What situation are you running into these package id specs existing that they should match the given package id?
From looking at the code, the implicit name is mostly to avoid redundancy, like if you depend on a package in the root of a git repo by the same name.
There was a problem hiding this comment.
In this issue:
Rustin170506/cargo-information#57
cargo info https://github.com/rust-lang/crates.io-index/clap
Updating crates.io index
error: could not find `https://github.com/rust-lang/crates.io-index/clap` in registry `https://github.com/rust-lang/crates.io-index`As you can see, it told me it could not find the clap crate from the crates.io.
I think this was a bug because the source ID doesn't have the package name in its URL. We shouldn't directly compare it.
There was a problem hiding this comment.
Where did you get that package id spec from?
$ cargo pkgid clap
https://github.com/rust-lang/crates.io-index#[email protected]There was a problem hiding this comment.
Oh. I copied it from here:
cargo/src/cargo/core/package_id_spec.rs
Line 41 in a5fa676
There was a problem hiding this comment.
Can I ask what is the difference between them? When to use the "https://crates.io/foo"?
There was a problem hiding this comment.
I wonder how representative that example is of real world spec ids...
There was a problem hiding this comment.
I will take a look at the details of the pkgid command. Thank you!
|
☔ The latest upstream changes (presumably #13128) made this pull request unmergeable. Please resolve the merge conflicts. |
What does this PR try to resolve?
close Rustin170506/cargo-information#57
We should not directly compare the package spec URL with the source ID URL because the package spec URL contains the package's name in the URL.
For example:
https://github.com/rust-lang/crates.io-index/clapUrl { scheme: "https", cannot_be_a_base: false, username: ", password: None, host: Some(Domain("github.com")), port: None, path: "/rust-lang/crates.io-index/clap", query: None, fragment: None }Url { scheme: "https", cannot_be_a_base: false, username: ", password: None, host: Some(Domain("github.com")), port: None, path: "/rust-lang/crates.io-index", query: None, fragment: None }How should we test and review this PR?
Could you check the unit tests? Before the fix, it could fail.
Additional information
Do we need to change the URL itself instead of the matching process?