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.

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

Allow passing a registry index url directly to cargo install #8344

Merged
merged 3 commits into from
Jun 14, 2020
Merged

Allow passing a registry index url directly to cargo install #8344

merged 3 commits into from
Jun 14, 2020

Conversation

kellda
Copy link
Contributor

@kellda kellda commented Jun 8, 2020

Fixes #8318

@rust-highfive
Copy link

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Jun 8, 2020

Thank you! I'm happily surprised that the diff is so small! It definitely needs a test or three, and a review from someone that knows the intricacies.

@kellda
Copy link
Contributor Author

kellda commented Jun 8, 2020

Could you please tell me where to add tests (integration or unit) and if possible tell me if there are similar tests for other commands that I can look at ?

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 8, 2020

Most of the install tests are in tests\testsuite\install.rs or in tests\testsuite\install_upgrade.rs

@ehuss
Copy link
Contributor

ehuss commented Jun 8, 2020

Thanks! I think instead of modifying existing tests, it would be best to add a new one. It can probably be more or less just like simple, minus the uninstall step which isn't really necessary.

@kellda
Copy link
Contributor Author

kellda commented Jun 8, 2020

Well I tried to do something like what's done for publish and search.
Currently no command have tests for --install on its own, so I'm not really sure what / how to test it properly.

Also I have some trouble testing locally right now, that doesn't help to write tests that passes

@ehuss
Copy link
Contributor

ehuss commented Jun 8, 2020

The current errors are due to the use of {reg}. It needs the format! macro to pass in the value, similar to this.

Otherwise, I would just copy the install::simple test as a new test, and add the --index flag.

If you're having trouble running tests, feel free to post what issues you're running into, I may be able to help.

@kellda
Copy link
Contributor Author

kellda commented Jun 8, 2020

The current errors are due to the use of {reg}. It needs the format! macro to pass in the value.

Yes I figured it out but didn't fixed it yet.

If you're having trouble running tests

Not sure you can help, here's my issues anyway:
Last things I tryed:

  • cargo build then cargo test --no-run -> error: could not compile cargo (says rustc failed because (signal: 9, SIGKILL: kill))
  • rerun cargo test --no-run -> compiled fine
  • cargo test -> most tests run fine, but testsuite fails: (signal: 4, SIGILL: illegal instruction)

So not really related to the test framework, I dunno if I messed up with apt-get or if it's some regression in rustc, or if something else went wrong with my linux install...

@ehuss
Copy link
Contributor

ehuss commented Jun 8, 2020

Oh, that's a tough one. I would try to get a core dump or attach a debugger for the situation that causes SIGKILL. Which rustc are you using? Did you install it with rustup?

SIGILL happens when there is a panic inside a panic.

@kellda
Copy link
Contributor Author

kellda commented Jun 8, 2020

I use rust stable 1.44.0 via rustup

@kellda
Copy link
Contributor Author

kellda commented Jun 8, 2020

Ok now it works, sorry for the multiples pushes and CI runs. I'll make sure I can run tests locally before my next PR

@ehuss
Copy link
Contributor

ehuss commented Jun 9, 2020

Thanks! However, as I mentioned, I think it would be ideal just to add a new test case instead of modifying the existing ones. I think one new test, based on simple, should be sufficient.

@kellda
Copy link
Contributor Author

kellda commented Jun 12, 2020

Is it OK now ?

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2020

Looks great, thanks! I pushed a small commit to update the man page.
@bors r+

@ehuss ehuss closed this Jun 14, 2020
@bors
Copy link
Collaborator

bors commented Jun 14, 2020

📌 Commit a0fb62f has been approved by ehuss

@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 Jun 14, 2020
@ehuss ehuss reopened this Jun 14, 2020
@bors
Copy link
Collaborator

bors commented Jun 14, 2020

⌛ Testing commit a0fb62f with merge 56636e9...

@bors
Copy link
Collaborator

bors commented Jun 14, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 56636e9 to master...

@bors bors merged commit 56636e9 into rust-lang:master Jun 14, 2020
@kellda kellda deleted the install-index-flag branch June 14, 2020 19:08
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 20, 2020
Update cargo

3 commits in 79c769c3d7b4c2cf6a93781575b7f592ef974255..089cbb80b73ba242efdcf5430e89f63fa3b5328d
2020-06-11 22:13:37 +0000 to 2020-06-15 14:38:34 +0000
- Support linker with -Zdoctest-xcompile. (rust-lang/cargo#8359)
- Fix doctests not running with --target=HOST. (rust-lang/cargo#8358)
- Allow passing a registry index url directly to `cargo install` (rust-lang/cargo#8344)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 22, 2020
Update cargo

3 commits in 79c769c3d7b4c2cf6a93781575b7f592ef974255..089cbb80b73ba242efdcf5430e89f63fa3b5328d
2020-06-11 22:13:37 +0000 to 2020-06-15 14:38:34 +0000
- Support linker with -Zdoctest-xcompile. (rust-lang/cargo#8359)
- Fix doctests not running with --target=HOST. (rust-lang/cargo#8358)
- Allow passing a registry index url directly to `cargo install` (rust-lang/cargo#8344)
@ehuss ehuss added this to the 1.46.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing a registry index url directly to cargo install
5 participants