WIP: Add normalized querykind Fixes #1079 #10985

tomharmon · 2022-08-13T22:33:38Z

What does this PR try to resolve?

See this thread, particularly this comment

How should we test and review this PR?

Currently trying to get a functional or ui test to work but am having issues.

If you don't know how to write and run your tests, please read the guide:
https://doc.crates.io/contrib/tests

Additional information

TODO:

tests
- UI Test: When I tried to write this test, I was having issues replicating the issue since I couldn't get the .cargo/config.toml file in tests/testsuite/<command>/<case>/in to be persisted. I have this test on another branch, add-normalized-querykind-test, which is just master + tests.
- Functional test, this is the output:

Click to expand

 ~/code/cargo/ [add-normalized-querykind+] cargo  test --test testsuite -- cargo_add_with_vendored_pkgs
> cargo test --test testsuite -- cargo_add_with_vendored_pkgs
   Compiling cargo v0.65.0 (/Users/thomasharmon/code/cargo)
    Finished test [unoptimized + debuginfo] target(s) in 3.64s
     Running tests/testsuite/main.rs (target/debug/deps/testsuite-a0b7d1f599e8fa04)

running 1 test
test cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages ... FAILED

failures:

---- cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages stdout ----
running `/Users/thomasharmon/code/cargo/target/debug/cargo vendor ./vendor`
running `/Users/thomasharmon/code/cargo/target/debug/cargo add cbindgen`
thread 'cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages' panicked at '
test failed running `/Users/thomasharmon/code/cargo/target/debug/cargo add cbindgen`
error: stdout did not match:
1        -    Updating crates.io index
2        -      Adding xml-rs v0.8.4 to dependencies.


other output:
warning: translating `cbindgen` to `xml-rs`
      Adding xml-rs v0.8.4 to dependencies.

', tests/testsuite/cargo_add_with_vendored_pkgs.rs:49:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2597 filtered out; finished in 29.65s

error: test failed, to rerun pass '--test testsuite'

- As described [in this issue](#10729 (comment)) rename `Fuzzy` to `Alternatives` for clarity

rust-highfive · 2022-08-13T22:33:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

weihanglo · 2022-08-14T19:02:12Z

I didn't look into the issue. Just remind that the test seems to perform real network requests. You may take existing tests as reference for setting up a fake registry and dependencies.

tomharmon · 2022-08-15T14:17:22Z

You're definitely right, I updated my test to reflect that. However I'm still having some issues which I need to debug in under to get a better understand to be able to ask the right questions. The test now replicates the original error and makes an assertion of what the correct output should be. It's still not passing though.

Right now I can setup a debugger in VSCode but don't know how to attach it to the cargo process itself so I can debug what's actually happening in the test (hope that makes sense). I know how to do it with CLion I think, so I'm going to try to buy the license for that so I can get the debugger going.

Thanks for the comment! I'll be back with a better understanding after I get the debugger running and attaching it to the actual cargo add process I'm testing, and not the test itself

epage · 2022-08-15T14:26:56Z

src/cargo/sources/directory.rs

-            QueryKind::Fuzzy => true,
+            QueryKind::Alternatives => true,
+            QueryKind::Normalized => {
+                let src_id = pkg.package_id().source_id();


Why the source check?

epage · 2022-08-15T14:28:26Z

tests/testsuite/cargo_add_with_vendored_pkgs.rs

+use cargo_test_support::{project, registry::Package};
+
+#[cargo_test]
+fn carg_add_with_vendored_packages() {


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)

epage · 2022-08-15T14:32:58Z

src/cargo/sources/registry/mod.rs

                let matched = match kind {
                    QueryKind::Exact => dep.matches(&s),
-                    QueryKind::Fuzzy => true,
+                    QueryKind::Alternatives => true,
+                    QueryKind::Normalized => true,


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.

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

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.

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.

epage · 2022-08-15T14:34:28Z

src/cargo/core/source/mod.rs

+    /// Behavior like `Exact` for path sources and like `Alternatives` for
+    /// registry sources


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.

tomharmon · 2022-08-15T14:38:38Z

Thank you @epage extensively for your extremely generous review and welcome in Zulip. I'll look into these things today and fix them up. This is awesome. I'm so excited to be learning and contributing to cargo so thanks so much for giving me some of your time. It's immensely helpful and appreciated. Cheers.

weihanglo · 2023-02-09T11:43:06Z

Ping @goodSyntax808. Checking in to see if you still got time to sort this out, or if you had any questions. Feel free to continue the discussion here or on Zuilp.

bors · 2023-04-06T17:29:22Z

☔ The latest upstream changes (presumably #11937) made this pull request unmergeable. Please resolve the merge conflicts.

tomharmon added 4 commits August 12, 2022 13:37

Rename QueryKind::Fuzzy to Alternatives

76fefef

- As described [in this issue](#10729 (comment)) rename `Fuzzy` to `Alternatives` for clarity

Use QueryKind::Nomarlize in cargo add

8699500

Add test for cargo add with vendored pkgs

21d72dd

Cargo fmt

cb8cbb8

rust-highfive assigned ehuss Aug 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022

Fix test to replicate error

19e506f

epage reviewed Aug 15, 2022

View reviewed changes

weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2022

tomharmon closed this by deleting the head repository Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add normalized querykind Fixes #1079 #10985

WIP: Add normalized querykind Fixes #1079 #10985

tomharmon commented Aug 13, 2022

rust-highfive commented Aug 13, 2022

weihanglo commented Aug 14, 2022

tomharmon commented Aug 15, 2022 •

edited

Loading

epage Aug 15, 2022

epage Aug 15, 2022

epage Aug 15, 2022

epage Aug 15, 2022

tomharmon Aug 16, 2022

tomharmon Aug 19, 2022

epage Aug 15, 2022

tomharmon commented Aug 15, 2022

weihanglo commented Feb 9, 2023

bors commented Apr 6, 2023

		/// Behavior like `Exact` for path sources and like `Alternatives` for
		/// registry sources

WIP: Add normalized querykind Fixes #1079 #10985

WIP: Add normalized querykind Fixes #1079 #10985

Conversation

tomharmon commented Aug 13, 2022

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Aug 13, 2022

weihanglo commented Aug 14, 2022

tomharmon commented Aug 15, 2022 • edited Loading

epage Aug 15, 2022

Choose a reason for hiding this comment

epage Aug 15, 2022

Choose a reason for hiding this comment

epage Aug 15, 2022

Choose a reason for hiding this comment

epage Aug 15, 2022

Choose a reason for hiding this comment

tomharmon Aug 16, 2022

Choose a reason for hiding this comment

tomharmon Aug 19, 2022

Choose a reason for hiding this comment

epage Aug 15, 2022

Choose a reason for hiding this comment

tomharmon commented Aug 15, 2022

weihanglo commented Feb 9, 2023

bors commented Apr 6, 2023

tomharmon commented Aug 15, 2022 •

edited

Loading