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

Jump to bottom

cargo install --git multiple packages with binaries found hint #11835

Merged
merged 4 commits into from
Mar 13, 2023
Merged

cargo install --git multiple packages with binaries found hint #11835

merged 4 commits into from
Mar 13, 2023

Conversation

domsleee
Copy link
Contributor

@domsleee domsleee commented Mar 12, 2023 •
edited
Loading

What does this PR try to resolve?

The issues discussed in: #4830

namely this one:

  1. a multiple packages with binaries found error should give users a hint about how to specify a single crate

I think improving the error message to provide a suggestion is a simple change that would help a lot of people when this happens, sorry if I'm out of line for just opening a PR here :)

Before

cargo 1.68.0 (115f345 2023-02-26)
image

After

image

Additional information

I added in a related test documenting existing behaviours

  • multiple_examples_error: "multiple packages with examples found" (i.e. not "binaries")

I added these tests which aren't necessarily related to this PR, but could be considered if the behaviour were to change

  • multiple_binaries_deep_select_uses_package_name: it uses the name, not the path. Is this a problem for crates with the same name?
  • multiple_binaries_in_selected_package_installs_all: so --bins is implied when a package is specified?
  • multiple_binaries_in_selected_package_with_bin_option_installs_only_one: demonstrates a use case where --bin and [crate] are both used

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2023

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) soon.

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2023
@domsleee domsleee changed the title Git install bin flag hint cargo install --git multiple packages with binaries found hint Mar 12, 2023
@ehuss
Copy link
Contributor

ehuss commented Mar 12, 2023

r? @weihanglo

@rustbot rustbot assigned weihanglo and unassigned ehuss Mar 12, 2023
weihanglo
weihanglo reviewed Mar 12, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thank you for filling in those test cases 👍🏾.

Just some minor style issues.

src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
tests/testsuite/install.rs Outdated Show resolved Hide resolved
src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
@weihanglo
Copy link
Member

Oops. Is this ready for review? I feel like it is 😅.

@domsleee domsleee marked this pull request as ready for review March 13, 2023 06:55
weihanglo
weihanglo approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Just a side note: the command suggested in the new error message could fail when a binary requiring additional features and a user just "copy-paste” it all. However, Cargo gives a good suggestion for missing features, and we also have an RFC proposal will mitigate this situation (rust-lang/rfcs#3374). I think it is a good enhancement without any major defect.

tests/testsuite/install.rs Outdated Show resolved Hide resolved
tests/testsuite/install.rs Outdated Show resolved Hide resolved
@domsleee
Copy link
Contributor Author

Thanks so much for the reviews 👍

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 13, 2023

📌 Commit 3363931 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2023
@bors
Copy link
Collaborator

bors commented Mar 13, 2023

⌛ Testing commit 3363931 with merge 4cce1f9...

bors added a commit that referenced this pull request Mar 13, 2023
@bors
Auto merge of #11835 - domsleee:git-install-bin-flag-hint, r=weihanglo
4cce1f9 
`cargo install --git` multiple packages with binaries found hint

### What does this PR try to resolve?
The issues discussed in: #4830

namely this one:
> 4. a multiple packages with binaries found error should give users a hint about how to specify a single crate

I think improving the error message to provide a suggestion is a simple change that would help a lot of people when this happens, sorry if I'm out of line for just opening a PR here :)

### Before
cargo 1.68.0 (115f345 2023-02-26)
![image](https://user-images.githubusercontent.com/14891742/224546157-d9b48bfd-f896-4fd1-9f0a-e04a3ad60ae2.png)

### After
![image](https://user-images.githubusercontent.com/14891742/224546182-d4b451ae-1b28-41b6-9d74-db860532512b.png)

### Additional information

I added in a related test documenting existing behaviours
* multiple_examples_error: "multiple packages with examples found" (i.e. not "binaries")

I added these tests which aren't necessarily related to this PR, but could be considered if the behaviour were to change
* `multiple_binaries_deep_select_uses_package_name`: it uses the name, not the path. Is this a problem for crates with the same name?
* `multiple_binaries_in_selected_package_installs_all`: so `--bins` is implied when a package is specified?
* `multiple_binaries_in_selected_package_with_bin_option_installs_only_one`: demonstrates a use case where `--bin` and `[crate]` are both used
@bors
Copy link
Collaborator

bors commented Mar 13, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 13, 2023
@weihanglo
Copy link
Member

Looks like a spurious failure due to network timeout not related to this PR.

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2023
@bors
Copy link
Collaborator

bors commented Mar 13, 2023

⌛ Testing commit 3363931 with merge 9855f92...

@bors
Copy link
Collaborator

bors commented Mar 13, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 9855f92 to master...

@bors bors merged commit 9855f92 into rust-lang:master Mar 13, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Mar 14, 2023
@weihanglo
Update cargo
3c7df56 
14 commits in 7d3033d2e59383fd76193daf9423c3d141972a7d..4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49
2023-03-08 17:05:08 +0000 to 2023-03-14 14:05:36 +0000
- ci: make clean-test-output a script for reuse (rust-lang/cargo#11848)
- Accurately show status when downgrading dependencies (rust-lang/cargo#11839)
- docs(contrib): Move Design Principles earlier in the book (rust-lang/cargo#11842)
- docs(contrib): Point compilation docs to doc comments (rust-lang/cargo#11841)
- `cargo install --git` multiple packages with binaries found hint (rust-lang/cargo#11835)
- Disable flaky auth tests when `gitoxide` runs them (rust-lang/cargo#11830)
- Add some documentation on writing cross-compilation tests (rust-lang/cargo#11825)
- chore: Use sparse protocol on stable CI (rust-lang/cargo#11829)
- Notice for potential unexpected shell expansions in help text of `cargo-add` (rust-lang/cargo#11826)
- Add tracking issue to gitoxide unstable docs (rust-lang/cargo#11822)
- Bump crates-io to 0.36.0 (rust-lang/cargo#11820)
- Bump to 0.71.0; update changelog (rust-lang/cargo#11815)
- docs(contrib): Move overview to lib (rust-lang/cargo#11809)
- Fix semver check for 1.68 (rust-lang/cargo#11817)
@weihanglo weihanglo mentioned this pull request Mar 14, 2023
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 15, 2023
@Dylan-DPC
Rollup merge of rust-lang#109150 - weihanglo:update-cargo, r=weihanglo
a08516b 
Update cargo

14 commits in 7d3033d2e59383fd76193daf9423c3d141972a7d..4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49
2023-03-08 17:05:08 +0000 to 2023-03-14 14:05:36 +0000
- ci: make clean-test-output a script for reuse (rust-lang/cargo#11848)
- Accurately show status when downgrading dependencies (rust-lang/cargo#11839)
- docs(contrib): Move Design Principles earlier in the book (rust-lang/cargo#11842)
- docs(contrib): Point compilation docs to doc comments (rust-lang/cargo#11841)
- `cargo install --git` multiple packages with binaries found hint (rust-lang/cargo#11835)
- Disable flaky auth tests when `gitoxide` runs them (rust-lang/cargo#11830)
- Add some documentation on writing cross-compilation tests (rust-lang/cargo#11825)
- chore: Use sparse protocol on stable CI (rust-lang/cargo#11829)
- Notice for potential unexpected shell expansions in help text of `cargo-add` (rust-lang/cargo#11826)
- Add tracking issue to gitoxide unstable docs (rust-lang/cargo#11822)
- Bump crates-io to 0.36.0 (rust-lang/cargo#11820)
- Bump to 0.71.0; update changelog (rust-lang/cargo#11815)
- docs(contrib): Move overview to lib (rust-lang/cargo#11809)
- Fix semver check for 1.68 (rust-lang/cargo#11817)

r? `@ghost`
@ehuss ehuss added this to the 1.70.0 milestone Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
5 participants
@domsleee @rustbot @ehuss @weihanglo @bors