Support uninstallation of multiple packages

[nossralf]

This is a WIP pull request with support for uninstalling multiple packages. It mirrors the logic used for cargo install.

Fixes #4560

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

[matklad]

Looks great @nossralf !

One question I have though is what is the interaction between --bin and multiple specs? Looks like currently passing --bin and several packages will try to remove the binary from all these packages, which does seem surprising...

Perhaps a safe option would be to forbid passing both --bin and several specs?

[nossralf]

Thanks! Yeah, that's the one thing I was uncertain of (I mentioned it in the issue I, perhaps redundantly, created for this, #4560).

The thing is, cargo install doesn't have any special handling for --bin in conjunction with multiple packages, which means it will just show errors for any packages that don't have binaries with that name, and install the ones that do.

I guess there is a case to be made for having install and uninstall effectively be mirrors of each other. Although I'm not sure if that speaks more to leaving uninstall like this or changing install to also have a check disallowing --bin when called with multiple packages?

I'll happily update the PR with such a check here. A question in that case, I suppose the check would happen in ops/cargo/cargo_install.rs as opposed to bin/uninstall.rs. From what I can tell, validation (besides docopt's automatic validation) happens in ops/, and then one uses bail! to exit.

[nossralf]

Updated the pull request to add an early bailout if --bin is given along with multiple packages. Also updated an existing test to use the new functionality, and added a new one to test the bailout.

The wording in the bailout message can probably be better.

[matklad]

Oh, sorry, this completely has fallen of my radar :(

Changes look good to me!

There were some concerns on the issue that we perhaps don't need this option. I personally feel like we should have at at least because install does, but let's ask @withoutboats and @alexcrichton as well.

Question 1: do we need (#4650 EDIT: #4560 of course) ?
Question 2: what is the peocedure for such small feature request? How should I decide if I can give an r+? How can a contributor be sure that an issue with a feature lablel is indeed intended to be fixed, and that the PR won't be turned down after the fact?

[nossralf]

Oh, no worries! I'm in no hurry.

I realize it's a gamble to submit a PR for a feature suggestion before it has been approved on the feature level. I would understand if the PR was closed without merging if the feature itself isn't approved for good reasons, that makes perfect sense. I think this change has merit, but understand if Cargo maintainers disagree. (I've personally still gotten value out of writing the code, since it's been good practice for me in learning Rust.)

Anyways, I had another test I wanted to add to this, and that's done now.

[alexcrichton]

This looks great to me, thanks @nossralf!

@matklad I think this is a fine feature to add, and in general we've been pretty lax about adding features to Cargo. If someone's motivated enough to send a PR that's typically enough to say "sure!". Overall though I tend to just figure that if it's "reasonable" (for whatever definition you'd like) it's good enough, and then canvassing the wider team failing that.

[withoutboats]

If someone's motivated enough to send a PR that's typically enough to say "sure!". Overall though I tend to just figure that if it's "reasonable" (for whatever definition you'd like) it's good enough, and then canvassing the wider team failing that.

I'd like to move away from this 😅 (esecially since features in cargo have tended to be insta-stable) but I still agree that in this case being symmetrical with install is closer to being a bug fix than a new feature.

[matklad]

Excellent, thanks @nossralf

@bors r+

[bors]

📌 Commit 05224d6 has been approved by matklad

[bors]

⌛ Testing commit 05224d6 with merge 42e031d...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 42e031d to master...