cargo install with specific yanked version gives confusing "not found" error #8565

pawanbisht62 · 2020-07-30T11:33:52Z

Resolves #8171

rust-highfive · 2020-07-30T11:33:55Z

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.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ehuss

Thanks! Looks like this needs a test. Let me know if you need any help writing one.

ehuss · 2020-07-31T14:47:16Z

src/cargo/ops/common_for_install_and_uninstall.rs

+        None => {
+            for pkg_id in deps.iter() {
+                if source.is_yanked(pkg_id.package_id()).unwrap_or(false) {
+                    bail!("provided package has been yanked `{}`", pkg_id.package_id())


Just a small note on the wording, perhaps it could be written as:

cannot install package `{}`, it has been yanked from {}

Thanks @ehuss for suggestion, I'll definitely update the message and can you please specify the value for the second placeholder here? I mean yanked from what? Correct me if I'm wrong, here you mean the package name and for first place holder the specific version of package right?
And regarding test it would be helpful, if you guide me to add test for this.

ehuss · 2020-07-31T18:47:46Z

For Cargo's testsuite, there is some high-level documentation in https://github.com/rust-lang/cargo/blob/master/CONTRIBUTING.md#running-tests for running tests. See https://github.com/rust-lang/cargo/blob/master/crates/cargo-test-support/src/lib.rs for an introduction to writing tests.

The test should probably go in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/install.rs. There are a bunch of other tests in there that you can look at for examples. The simple test up top is a good, basic example.

Essentially it needs to call Package::new to create a new package in the registry, and call yanked to set its yanked status.

Then call cargo_process("install foo") and check that it fails with_status(101) and that check the output using with_stderr("...").

bishtpawan · 2020-08-01T17:14:27Z

Hi @ehuss,I've updated the PR by updating the code and also added one new test-case.
Can you please have a look at the CI/test the failed one. I'm unable to get details like why 2 tests are failing. Thanks

ehuss · 2020-08-03T18:08:53Z

tests/testsuite/install.rs

+    Package::new("baz", "0.0.1").yanked(true).publish();
+    cargo_process("install baz --version 0.0.1")
+        .with_status(101)
+        .with_stderr_contains("error: cannot install package `baz`, it has been yanked from registry `https://github.com/rust-lang/crates.io-index`")


Can you split up this line so it is less than 100 characters long? Just add some \ characters to split the string across a few lines.

Thanks for the suggestion. I've updated the PR, please have a look.

ehuss · 2020-08-03T18:09:19Z

tests/testsuite/install_upgrade.rs

-[ERROR] could not find `foo` in registry `[..]` with version `=1.0.1`
-",
+        .with_stderr_contains(
+            "error: cannot install package `foo`, it has been yanked from registry `https://github.com/rust-lang/crates.io-index`",


This string could be split up, too.

Thanks for the suggestion. I've updated the PR, please have a look.

ehuss · 2020-08-03T18:21:12Z

src/cargo/ops/common_for_install_and_uninstall.rs

-        ),
+        None => {
+            let version: String = dep.version_req().to_string();
+            match PackageId::new(dep.package_name(), &version[1..], source.source_id()) {


I'm a little uncomfortable just blindly stripping the first character from the version req. Perhaps only do the check if version_req().is_exact()? I think that should guarantee that it is = followed by an exact version.

Which reminds me that this doesn't help for non-specific versions (like * or ~2). That's probably a bigger can of worms, though.

ehuss · 2020-08-03T18:26:09Z

src/cargo/ops/common_for_install_and_uninstall.rs

+                    }
+                }
+                Err(_) => bail!(
+                    "could not find `{}` in {} with version `{}`",


Can this be restructured to avoid the duplication of these two cases? Maybe something like:

let maybe_pkg_id = PackageId::new(dep.package_name(), &version[1..], source.source_id()); let is_yanked = maybe_pkg_id.map_or(false, |pkg_id| source.is_yanked(pkg_id).unwrap_or(false)); if is_yanked { // ...yanked error } else { // ...not found error }

Yes, you are correct. Thanks for the snippet. I've incorporated this in the PR. Please have a look once you get time.

ehuss · 2020-08-03T18:27:51Z

Can you please have a look at the CI/test the failed one. I'm unable to get details like why 2 tests are failing. Thanks

This was just something that changed in nightly, it should be fixed now (via #8576). If you rebase on the latest master, it should be fixed.

ehuss · 2020-08-05T15:08:58Z

src/cargo/ops/common_for_install_and_uninstall.rs

+            if dep.version_req().is_exact() {
+                pkg_id = PackageId::new(dep.package_name(), &version[1..], source.source_id());
+            } else {
+                pkg_id = PackageId::new(dep.package_name(), &version[..], source.source_id());


Since this line is almost guaranteed to fail, I think it is a little misleading to try to create the PackageId here. Maybe be a little more explicit here? For example, something like the following:

let is_yanked = if dep.version_req().is_exact() { let version: String = dep.version_req().to_string(); PackageId::new(dep.package_name(), &version[1..], source.source_id()) .map_or(false, |pkg_id| source.is_yanked(pkg_id).unwrap_or(false)) } else { false };

okay got it, like if we don't have exact version then control will always go to the else part. Let me update the PR. Thanks again for explaining your point with the code snippet, which really helps me to get your point.

ehuss · 2020-08-05T20:31:02Z

Thanks! And thanks for putting up with my pickiness. 😄

@bors r+

bors · 2020-08-05T20:31:04Z

📌 Commit 81687e7 has been approved by ehuss

bors · 2020-08-05T20:31:13Z

⌛ Testing commit 81687e7 with merge 0aedda2...

bors · 2020-08-05T20:54:44Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0aedda2 to master...

Update cargo 7 commits in 1653f354644834073d6d2541e27fae94588e685e..ab32ee88dade1b50c77347599e82ca2de3fb8a51 2020-08-04 23:14:37 +0000 to 2020-08-10 17:44:43 +0000 - Build manpage archive deterministically (rust-lang/cargo#8600) - doc: Qualify GNU licenses in example license field (rust-lang/cargo#8604) - Fix jobserver_exists test on single-cpu systems (rust-lang/cargo#8598) - Fix small typo in reference/profiles.md (rust-lang/cargo#8605) - Default cargo publish to the alt registry if it's the only allowed one (rust-lang/cargo#8571) - cargo install with specific yanked version gives confusing "not found" error (rust-lang/cargo#8565) - Fix typo (rust-lang/cargo#8589)

rust-highfive assigned ehuss Jul 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2020

pawanbisht62 force-pushed the master branch from ee368c3 to 31316a9 Compare July 30, 2020 11:38

pawanbisht62 changed the title ~~cargo install with specific yanked version gives confusing "not found" error~~ [WIP] cargo install with specific yanked version gives confusing "not found" error Jul 30, 2020

pawanbisht62 changed the title ~~[WIP] cargo install with specific yanked version gives confusing "not found" error~~ cargo install with specific yanked version gives confusing "not found" error Jul 31, 2020

ehuss reviewed Jul 31, 2020

View reviewed changes

pawanbisht62 force-pushed the master branch 5 times, most recently from 4c20b3b to faf3ca5 Compare August 1, 2020 16:58

ehuss reviewed Aug 3, 2020

View reviewed changes

bishtpawan added 5 commits August 5, 2020 17:31

Add yank check for package that needs to be download

8bea1b0

Update approach to check for yanked version

c180eb4

Add test case for yank validation check

844cde2

Refactor code

a50b10a

Add pattern match for yank check and updated corresponding test cases

dd654d5

pawanbisht62 force-pushed the master branch 2 times, most recently from e8d8a5a to 538fd80 Compare August 5, 2020 12:13

Refactor code and resolve PR comments

3952fdb

pawanbisht62 force-pushed the master branch from 538fd80 to 3952fdb Compare August 5, 2020 12:17

ehuss reviewed Aug 5, 2020

View reviewed changes

Resolve PR comments

81687e7

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2020

bors merged commit 0aedda2 into rust-lang:master Aug 5, 2020

ehuss mentioned this pull request Aug 11, 2020

Update cargo rust-lang/rust#75426

Merged

wonderfulspam mentioned this pull request Sep 7, 2020

Can't install ripgrep_all with cargo from crates phiresky/ripgrep-all#67

Closed

ehuss added this to the 1.47.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo install with specific yanked version gives confusing "not found" error #8565

cargo install with specific yanked version gives confusing "not found" error #8565

pawanbisht62 commented Jul 30, 2020

rust-highfive commented Jul 30, 2020

ehuss left a comment

ehuss Jul 31, 2020

pawanbisht62 Jul 31, 2020 •

edited

Loading

ehuss commented Jul 31, 2020

bishtpawan commented Aug 1, 2020

ehuss Aug 3, 2020

bishtpawan Aug 5, 2020

ehuss Aug 3, 2020

bishtpawan Aug 5, 2020

ehuss Aug 3, 2020

ehuss Aug 3, 2020

bishtpawan Aug 5, 2020

ehuss commented Aug 3, 2020

ehuss Aug 5, 2020

bishtpawan Aug 5, 2020

ehuss commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

cargo install with specific yanked version gives confusing "not found" error #8565

cargo install with specific yanked version gives confusing "not found" error #8565

Conversation

pawanbisht62 commented Jul 30, 2020

rust-highfive commented Jul 30, 2020

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawanbisht62 Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

ehuss commented Jul 31, 2020

bishtpawan commented Aug 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

pawanbisht62 Jul 31, 2020 •

edited

Loading