Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Aug 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8bea1b0
Add yank check for package that needs to be download
bishtpawan Jul 30, 2020
c180eb4
Update approach to check for yanked version
bishtpawan Jul 31, 2020
844cde2
Add test case for yank validation check
bishtpawan Aug 1, 2020
a50b10a
Refactor code
bishtpawan Aug 1, 2020
dd654d5
Add pattern match for yank check and updated corresponding test cases
bishtpawan Aug 1, 2020
3952fdb
Refactor code and resolve PR comments
bishtpawan Aug 5, 2020
81687e7
Resolve PR comments
bishtpawan Aug 5, 2020
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
31 changes: 25 additions & 6 deletions src/cargo/ops/common_for_install_and_uninstall.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,12 +544,31 @@ where
let pkg = Box::new(source).download_now(pkgid, config)?;
Ok(pkg)
}
None => bail!(
"could not find `{}` in {} with version `{}`",
dep.package_name(),
source.source_id(),
dep.version_req(),
),
None => {
let version: String = dep.version_req().to_string();
let pkg_id;
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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
};

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
let is_yanked =
pkg_id.map_or(false, |pkg_id| source.is_yanked(pkg_id).unwrap_or(false));
if is_yanked {
bail!(
"cannot install package `{}`, it has been yanked from {}",
dep.package_name(),
source.source_id()
)
} else {
bail!(
"could not find `{}` in {} with version `{}`",
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

dep.package_name(),
source.source_id(),
dep.version_req(),
)
}
}
}
}

Expand Down
21 changes: 17 additions & 4 deletions tests/testsuite/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@ use std::io::prelude::*;

use cargo_test_support::cross_compile;
use cargo_test_support::git;
use cargo_test_support::install::{
assert_has_installed_exe, assert_has_not_installed_exe, cargo_home,
};
use cargo_test_support::paths;
use cargo_test_support::registry::{registry_path, registry_url, Package};
use cargo_test_support::{
basic_manifest, cargo_process, no_such_file_err_msg, project, symlink_supported, t,
};

use cargo_test_support::install::{
assert_has_installed_exe, assert_has_not_installed_exe, cargo_home,
};
use cargo_test_support::paths;

fn pkg(name: &str, vers: &str) {
Package::new(name, vers)
.file("src/lib.rs", "")
Expand Down Expand Up @@ -1555,3 +1556,15 @@ fn install_git_with_symlink_home() {
)
.run();
}

#[cargo_test]
fn install_yanked_cargo_package() {
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`",
)
.run();
}
8 changes: 3 additions & 5 deletions tests/testsuite/install_upgrade.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,11 +799,9 @@ fn already_installed_updates_yank_status_on_upgrade() {

cargo_process("install foo --version=1.0.1")
.with_status(101)
.with_stderr(
"\
[UPDATING] `[..]` index
[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`",
)
.run();

Expand Down