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

Error when precise without -p flag #11349

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Error when precise without -p flag #11349

merged 3 commits into from
Nov 30, 2022

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Nov 8, 2022 •
edited
Loading

What does this PR try to resolve?

close #10919

Follow up #10988, see #10919 (comment)

How should we test and review this PR?

You can manually build and test it. You can try cargo update --precise xxx without -p flag.

Additional information

It has already been released on stable. rustc 1.65.0 (897e37553 2022-11-02)

@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2022

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2022
@Rustin170506
Update tests
71e4dbd 
Signed-off-by: hi-rustin <[email protected]>
@Rustin170506 Rustin170506 force-pushed the rustin-patch-error-cli branch 2 times, most recently from 381f9c8 to 71e4dbd Compare November 8, 2022 02:00
weihanglo
weihanglo approved these changes Nov 23, 2022
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 good!

I saw it landed in the current stable (1.65), just not sure how long the transition period should be. Given people don't run cargo update daily, do we want to wait one or two more versions?

Anyway, I can bring this to the next triage meeting.

weihanglo
weihanglo reviewed Nov 29, 2022
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.

Hello. Cargo team talked about this today. We think it's fine to make it a hard error for --precise without -p, as --precise takes a value that is only meaningful with -p.

For --aggressive, we tend not to turn it an error. --aggressive is more like the default behaviour of cargo update without any options. We had a debate on whether to keep the warning. We may want to emit a warning like "redundant option --aggressive. cargo update without specifying a package implies --aggressive by default". However, it is a bit lengthy and not really useful. I think it's fine to remove the entire warning and relevant tests as well.

tests/testsuite/update.rs Outdated Show resolved Hide resolved
tests/testsuite/update.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

Hello. Cargo team talked about this today. We think it's fine to make it a hard error for --precise without -p, as --precise takes a value that is only meaningful with -p.

For --aggressive, we tend not to turn it an error. --aggressive is more like the default behaviour of cargo update without any options. We had a debate on whether to keep the warning. We may want to emit a warning like "redundant option --aggressive. cargo update without specifying a package implies --aggressive by default". However, it is a bit lengthy and not really useful. I think it's fine to remove the entire warning and relevant tests as well.

Make sense. Thanks for bringing this to the meeting!
Thanks for your review! 💚 💙 💜 💛 ❤️

@Rustin170506 Rustin170506 changed the title Error when precise or aggressive without -p flag Error when precise without -p flag Nov 30, 2022
weihanglo
weihanglo approved these changes Nov 30, 2022
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 update!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 30, 2022

📌 Commit c51f8ad 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 Nov 30, 2022
@bors
Copy link
Contributor

bors commented Nov 30, 2022

⌛ Testing commit c51f8ad with merge 7b9069e...

@bors
Copy link
Contributor

bors commented Nov 30, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 7b9069e to master...

@bors bors merged commit 7b9069e into rust-lang:master Nov 30, 2022
@weihanglo
Copy link
Member

In addition, if you have the access of new GitHub code search, here is the search result showing a non-neglectable amount of usage of cargo update --aggressive without a -p flag.

weihanglo added a commit to weihanglo/rust that referenced this pull request Dec 3, 2022
@weihanglo
Update cargo
9bd6062 
9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306
2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000
- Refactor generate_targets into separate module (rust-lang/cargo#11445)
- Improve file found in multiple build targets warning (rust-lang/cargo#11299)
- Error when precise without -p flag (rust-lang/cargo#11349)
- Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430)
- Aware of compression ratio for unpack size limit (rust-lang/cargo#11337)
- Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403)
- Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400)
- Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387)
- Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
@weihanglo weihanglo mentioned this pull request Dec 3, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2022
@bors
Auto merge of rust-lang#105073 - weihanglo:update-cargo, r=weihanglo
dd7a125 
Update cargo

9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306
2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000
- Refactor generate_targets into separate module (rust-lang/cargo#11445)
- Improve file found in multiple build targets warning (rust-lang/cargo#11299)
- Error when precise without -p flag (rust-lang/cargo#11349)
- Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430)
- Aware of compression ratio for unpack size limit (rust-lang/cargo#11337)
- Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403)
- Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400)
- Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387)
- Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
@ehuss ehuss added this to the 1.67.0 milestone Dec 14, 2022
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.67.0
Development

Successfully merging this pull request may close these issues.

cargo update --precise without -p flag silently succeeds and runs cargo update
5 participants
@Rustin170506 @rustbot @weihanglo @bors @ehuss