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

Add an error when no crates are found from cargo search #11038

Closed
wants to merge 1 commit into from
Closed

Add an error when no crates are found from cargo search #11038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion src/cargo/ops/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::task::Poll;
use std::time::Duration;
use std::{cmp, env};

use anyhow::{bail, format_err, Context as _};
use anyhow::{anyhow, bail, format_err, Context as _};
use cargo_util::paths;
use crates_io::{self, NewCrate, NewCrateDependency, Registry};
use curl::easy::{Easy, InfoType, SslOpt, SslVersion};
Expand Down Expand Up @@ -984,6 +984,14 @@ pub fn search(
)
})?;

if total_crates == 0 {
return Err(anyhow!(
Copy link
Contributor

Choose a reason for hiding this comment

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

The motivation for this is scripting but we don't provide any programmatic output which makes me feel like we are providing a false promise by improving scripting support.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the programmatic output is the error code in this case. It should be fairly trivial to check what the code is in most scripting languages.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is speculation. I'm concerned that there is also parsing of the output (why do a cargo search and only care about the exit code?). We should wait on an actual reply from the reporter

"could not find any crates matching `{}` from the registry at {}",
query,
registry.host()
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we elide the registry if its crates.io

));
}
Comment on lines +987 to +993
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you expand on why you feel this behavior change is acceptable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely certain that an error is the correct answer, I mostly did that as it was requested. I think an error might be too harsh since there really wasn't an "error", just no matches. Keeping that in mind, an error is the easiest way to change the output code (1.61.0 does allow main to return different codes which could be a solution).

That being said, something should probably be shown when no crates were found. Having it output nothing seems like a misstep.

Copy link
Contributor

Choose a reason for hiding this comment

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

1.61.0 does allow main to return different codes which could be a solution).

There is also std::process::exit (generally not good to do in a library call though)

Having it output nothing seems like a misstep.

At least apt search doesn't really say anything.


let names = crates
.iter()
.map(|krate| format!("{} = \"{}\"", krate.name, krate.max_version))
Expand Down
26 changes: 26 additions & 0 deletions tests/testsuite/search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,3 +162,29 @@ fn colored_results() {
.with_stdout_contains("[..]\x1b[[..]")
.run();
}

#[cargo_test]
fn no_results() {
let _server = RegistryBuilder::new()
.http_api()
.add_responder("/api/v1/crates", |_| Response {
code: 200,
headers: vec![],
body: br#"
{
"crates": [],
"meta": {
"total": 0
}
}
"#
.to_vec(),
})
.build();
cargo_process("search z12345")
.with_status(101)
.with_stderr_contains(
"[ERROR] could not find any crates matching `z12345` from the registry at [..]",
)
.run();
}