Show description for user-installed binaries when running "cargo --list" #13847
Conversation
Also added a description method for the Registry struct in crates/crates-io/lib.rs and change submodule's "common_for_install_uninstall" visibility to public. All these changes were necessary in order to implement the forementioned feature
|
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) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-cli
There was a problem hiding this comment.
Thanks for the contribution.
There is an issue tracking this #10662. Per the contributing guide, we'd like to have a design discussion before diving into implementation details.
crates/crates-io/lib.rs
Outdated
| @@ -393,6 +414,8 @@ impl Registry { | |||
| headers.append("Content-Type: application/json")?; | |||
| } | |||
|
|
|||
| headers.append(&format!("User-Agent: cargo-cli"))?; | |||
There was a problem hiding this comment.
This is not ideal. Cargo has its own user-agent header when communicating with crates.io. This kind of setting should be in cargo main crate, not crates-io. Also there is curl::easy::Easy::useragent you could use. `
There was a problem hiding this comment.
Yeah, I did some searching around and found that in cargo::util::network::http exists a function named http_handle that autoconfigures things like the user-agent, etc. based on the gctx (GlobalContext) variable. I assume it is ok to use that?
src/bin/cargo/cli.rs
Outdated
| .cloned(); | ||
|
|
||
| if let Some(installed_by) = installed_by { | ||
| let desc = registry.description(&installed_by).ok().flatten(); |
There was a problem hiding this comment.
Personally, I feel like it's overkill talking over the network for descriptions. Yet, let's back to design discussion first.
There was a problem hiding this comment.
Querying the network for such little things is overkill indeed. However, from what I've found,currently no other better way to do this exists.
Perhaps what could be done here is implement some form of cache to store binaries' descriptions so that we don't have to query them each time.
|
I might have found why those tests are failing. For some reason, by making those changes to the code, |
|
I am not going to discuss any specific implementation further until we have a conclusion on the design. Could we bring it back to #10662, and gather ideas there? |
weihanglo
added
Command-install
S-needs-design
|
I'm going to close per the comment about about not having a chosen design, yet. Additionally, I don't think we will want to do network requests in the |
What does this PR try to resolve?
I've noticed that when running
cargo --list, user-installed subcommands wouldn't have a description. In order to resolve this, cargo will now communicate with the crates.io registry (if those packages were installed from crates.io ofc) and retrieve the package description from there. This also works if the binary name is different than the name of the package (for example, if a package installs multiple binaries)How should we test and review this PR?
I've noticed that some test use the
--listflag, so if they pass, this should be fineAdditional information
crates.io allows someone to go to
https://crates.io/api/v1/crates/{crate}and get info about this crate in a JSON format. However, from what I've found, this isn't standardized for other Rust registries, and so, if a subcommand was installed from another registry, it won't print a description. Please verify that this is the case. If not, I can remove the relevant checks for other registries.IMPORTANT
I haven't fixed the relevant tests yet, so DON'T merge this immediately