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

Fix cargo install --index when used with registry.default #11302

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Fix cargo install --index when used with registry.default #11302

merged 1 commit into from
Nov 16, 2022

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Oct 27, 2022

Setting registry.default causes the args.registry call to return the default registry as if it were passed through --registry, which leaves the --index argument ignored in cargo install, since registry is checked first.

Fixes #11301 by checking for index before registry.

Note that if you try to pass both --index and --registry, then a command-line parser error (correctly) occurs:

The argument '--registry <REGISTRY>' cannot be used with '--index <INDEX>'

@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 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 Oct 27, 2022
@Eh2406
Copy link
Contributor

Eh2406 commented Oct 27, 2022

You have pointed out before that registry.default is woefully under tested. Would this be a good PR to try to fix that?

weihanglo
weihanglo approved these changes Oct 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.

This looks fine. And I agree with Eh2406. We might want some tests against registry.default. Or at least posting an issue calling out for help.

@weihanglo
Copy link
Member

A bit off-topic. I wonder if --index and --registry are mutually exclusive1 throughout all commands in Cargo. If that is true, maybe we should have an enum combining them during arg parsing instead passing down two separate Option<&str>.

Footnotes

  1. At least for some registry operations, it seems to be the case. https://github.com/rust-lang/cargo/blob/67398aec95a7eccb0cfa2347fc60dcbcdd597b6c/src/cargo/ops/registry.rs#L1032-L1039 ↩

@weihanglo
Copy link
Member

Friendly ping @arlosi. Do you want to add a test for this particular scenario? A test following steps described in #11301 seems fine.

@arlosi
Copy link
Contributor Author

arlosi commented Nov 15, 2022

Added a test and changed the implementation strategy slightly.

There are complications when trying to add a new RegistryOrIndex enum type. --registry and --index do seem to be mutually exclusive, but they're not always both used. For example cargo new only uses --registry and cargo publish does additional processing of the registry value based on the publish field in Cargo.toml (if set).

weihanglo
weihanglo approved these changes Nov 16, 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.

I am not entirely sold but yup we can proceed first. I'll create an issue for exploring a way to make it more future-proof. Thank you for fixing this :)

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2022

📌 Commit 24bf873 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 16, 2022
@bors
Copy link
Contributor

bors commented Nov 16, 2022

⌛ Testing commit 24bf873 with merge b690ab4...

@bors
Copy link
Contributor

bors commented Nov 16, 2022

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

@bors bors merged commit b690ab4 into rust-lang:master Nov 16, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 18, 2022
@weihanglo
Update cargo
35f8962 
3 commits in 16b097879b6f117c8ae698aab054c87f26ff325e..eb5d35917b2395194593c9ca70c3778f60c1573b
2022-11-14 23:28:16 +0000 to 2022-11-17 22:08:43 +0000
- Fix several tests that are waiting 60 seconds for publishing to time out (rust-lang/cargo#11388)
- Implement RFC 3139: alternative registry authentication support (rust-lang/cargo#10592)
- Fix cargo install --index when used with registry.default (rust-lang/cargo#11302)
@weihanglo weihanglo mentioned this pull request Nov 18, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#104587 - weihanglo:update-cargo, r=weihanglo
c916a8d 
Update cargo

3 commits in 16b097879b6f117c8ae698aab054c87f26ff325e..eb5d35917b2395194593c9ca70c3778f60c1573b
2022-11-14 23:28:16 +0000 to 2022-11-17 22:08:43 +0000
- Fix several tests that are waiting 60 seconds for publishing to time out (rust-lang/cargo#11388)
- Implement RFC 3139: alternative registry authentication support (rust-lang/cargo#10592)
- Fix cargo install --index when used with registry.default (rust-lang/cargo#11302)

r? `@ghost`
@ehuss ehuss added this to the 1.67.0 milestone Dec 30, 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 install ignores --index when registry.default is set
6 participants
@arlosi @rustbot @Eh2406 @weihanglo @bors @ehuss