Skip to content

WIP: Add normalized querykind Fixes #1079 #10985

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ pub(super) fn activation_error(
// Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`
// was meant. So we try asking the registry for a `fuzzy` search for suggestions.
let mut candidates = loop {
match registry.query_vec(&new_dep, QueryKind::Fuzzy) {
match registry.query_vec(&new_dep, QueryKind::Alternatives) {
Poll::Ready(Ok(candidates)) => break candidates,
Poll::Ready(Err(e)) => return to_resolve_err(e),
Poll::Pending => match registry.block_until_ready() {
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ pub enum QueryKind {
/// Each source gets to define what `close` means for it.
/// Path/Git sources may return all dependencies that are at that URI,
/// whereas an `Index` source may return dependencies that have the same canonicalization.
Fuzzy,
Alternatives,
/// Behavior like `Exact` for path sources and like `Alternatives` for
/// registry sources
Comment on lines +120 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

Please focus on the desired behavior, and not the implementation, for the comment.

This one will match a dependency in all ways except it will normalize the package name. Each source defines what normalizing means.

Normalized,
}

pub enum MaybePackage {
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/ops/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ fn get_latest_dependency(
}
MaybeWorkspace::Other(query) => {
let possibilities = loop {
match registry.query_vec(&query, QueryKind::Fuzzy) {
match registry.query_vec(&query, QueryKind::Normalized) {
std::task::Poll::Ready(res) => {
break res?;
}
Expand Down Expand Up @@ -497,7 +497,7 @@ fn select_package(
MaybeWorkspace::Other(query) => {
let possibilities = loop {
// Exact to avoid returning all for path/git
match registry.query_vec(&query, QueryKind::Exact) {
match registry.query_vec(&query, QueryKind::Normalized) {
std::task::Poll::Ready(res) => {
break res?;
}
Expand Down Expand Up @@ -611,7 +611,7 @@ fn populate_available_features(
MaybeWorkspace::Other(query) => query,
};
let possibilities = loop {
match registry.query_vec(&query, QueryKind::Fuzzy) {
match registry.query_vec(&query, QueryKind::Normalized) {
std::task::Poll::Ready(res) => {
break res?;
}
Expand Down
10 changes: 9 additions & 1 deletion src/cargo/sources/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,15 @@ impl<'cfg> Source for DirectorySource<'cfg> {
let packages = self.packages.values().map(|p| &p.0);
let matches = packages.filter(|pkg| match kind {
QueryKind::Exact => dep.matches(pkg.summary()),
QueryKind::Fuzzy => true,
QueryKind::Alternatives => true,
QueryKind::Normalized => {
let src_id = pkg.package_id().source_id();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the source check?

if src_id.is_path() {
dep.matches(pkg.summary())
} else {
true
}
}
});
for summary in matches.map(|pkg| pkg.summary().clone()) {
f(summary);
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,8 @@ impl<'cfg> Source for PathSource<'cfg> {
for s in self.packages.iter().map(|p| p.summary()) {
let matched = match kind {
QueryKind::Exact => dep.matches(s),
QueryKind::Fuzzy => true,
QueryKind::Alternatives => true,
QueryKind::Normalized => dep.matches(s),
};
if matched {
f(s.clone())
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,8 @@ impl<'cfg> Source for RegistrySource<'cfg> {
.query_inner(dep, &mut *self.ops, &self.yanked_whitelist, &mut |s| {
let matched = match kind {
QueryKind::Exact => dep.matches(&s),
QueryKind::Fuzzy => true,
QueryKind::Alternatives => true,
QueryKind::Normalized => true,
Comment on lines 740 to +743
Copy link
Contributor

Choose a reason for hiding this comment

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

The test now replicates the original error and makes an assertion of what the correct output should be. It's still not passing though.

Put an assert in here and run your tests. My guess is this code isn't being hit.

My guess is we are "publishing" the packages to a directory registry which will be hitting the directory.rs code path rather than the registry/mod.rs code path.

maybe if we setup a sparse/http registry in the test, we could reproduce this.

Another alternative is we go past the MVP solution and implement normalization for all sources.

The final alternative is that we don't have a test for this. As the note above hints at, we are only testing one source type's normalization and there would be two main classes of normalization here. Unless we went and verified all sources, there isn't really much of a difference between implementing the feature for a different code path and testing it vs just not testing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

note: also as part of characterizing this, have you been able to reproduce the expected behavior by hand by running cargo run -- add ...?

Copy link
Author

Choose a reason for hiding this comment

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

So both the test and running by hand via ../<snip>/target/debug/cargo add ... are hitting the code in src/cargo/sources/registry/mod.rs actually. I am not able to reproduce the expected behavior by hand, I still get the original error. So seems like I am missing something in the implementation here. I need to actually dig into it more and get some context which might take me a day or two more. I'll try to come back with a solution or a pointed question.

Copy link
Author

Choose a reason for hiding this comment

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

Update: Haven't given up at all, but had to pack up my family members house being sold this week. Will have time to dig in this weekend.

};
if matched {
f(s);
Expand Down
52 changes: 52 additions & 0 deletions tests/testsuite/cargo_add_with_vendored_pkgs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use cargo_test_support::{project, registry::Package};

#[cargo_test]
fn carg_add_with_vendored_packages() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put this in with the other cargo add tests (tests/testsuite/cargo_add) and look to the tests in there for how they should be written (its using the document UI test pattern)

let p = project()
.file("src/lib.rs", "")
.file(
"Cargo.toml",
r#"[package]
name = "example"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
xml-rs = "0.8.4""#,
)
.build();
Package::new("xml-rs", "0.8.4").publish();
Package::new("cbindgen", "0.9.4").publish();

p.cargo("vendor ./vendor")
.with_stdout(
r#"
[source.crates-io]
replace-with = "vendored-sources"

[source.vendored-sources]
directory = "./vendor""#,
)
.run();
p.change_file(
".cargo/config",
r#"
[source.crates-io]
replace-with = "vendored-sources"

[source.vendored-sources]
directory = "./vendor""#,
);
p.cargo("add cbindgen")
// current output
//.with_stdout(r#"warning: translating `cbindgen` to `xml-rs`
//Adding xml-rs v0.8.4 to dependencies."#)
// correct output
.with_stdout(
r#" Updating crates.io index
Adding xml-rs v0.8.4 to dependencies."#,
)
.run();
}
1 change: 1 addition & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ mod build_script_env;
mod build_script_extra_link_arg;
mod cache_messages;
mod cargo_add;
mod cargo_add_with_vendored_pkgs;
mod cargo_alias_config;
mod cargo_command;
mod cargo_config;
Expand Down