Make "cargo uninstall" uninstall the cwd bins #5927
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
src/cargo/ops/cargo_install.rs
Outdated
| Entry::Occupied(e) => e, | ||
| Entry::Vacant(..) => panic!("entry not found: {}", result), | ||
| Entry::Vacant(..) => panic!("entry not found: {}", pkgid), |
There was a problem hiding this comment.
This panic happens if you attempt to cargo uninstall something that is not installed.
There was a problem hiding this comment.
This is the status quo despite this change. What change are you requesting?
There was a problem hiding this comment.
I think previously this code path should have been unreachable. The call to query_str ensured that a match was found. Since it no longer uses a query, I think it probably should just return a normal cargo error. Call bail! with a message indicating that the package is not installed ("package `blah` is not installed" or something along those lines).
src/cargo/ops/cargo_install.rs
Outdated
| let crate_metadata = metadata(config, root)?; | ||
| let metadata = read_crate_list(&crate_metadata)?; | ||
| let source_id = SourceId::for_path(config.cwd())?; | ||
| let (pkg, _source) = select_path_pkg(None, &source_id, None, config, true)?; |
There was a problem hiding this comment.
This includes an error message that mentions --path and --git which don't exist in uninstall.
|
Thanks for the review, @ehuss! I've address one of your comments, but I'm unsure about the other. 😕 |
dwijnand
force-pushed
the
uninstall-cwd
branch
from
fd9fbf8
to
52728d4
Compare
|
@ehuss I spent some time reproducing the test case you predicted so now we can iterate on how to handle it better. |
dwijnand
force-pushed
the
uninstall-cwd
branch
from
f3eac87
to
8921abd
Compare
|
All done from me, ready for re-review. |
|
This is failing on Travis CI for: which I'd say is unrelated to this PR. 😕 |
dwijnand
force-pushed
the
uninstall-cwd
branch
from
2c0472d
to
d87951a
Compare
|
LGTM, r+ unless anyone else has input. |
|
@bors: r=ehuss |
|
📌 Commit d87951a has been approved by |
Make "cargo uninstall" uninstall the cwd bins Fixes #5916 Tested with a local build of cargo, using coreutils: 17:33:57 $ dcargo uninstall Removing /Users/dnw/.cargo/bin/uutils
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #5916
Tested with a local build of cargo, using coreutils: