-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
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. |
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. |
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 ? |
Most of the install tests are in tests\testsuite\install.rs or in tests\testsuite\install_upgrade.rs |
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 |
Well I tried to do something like what's done for Also I have some trouble testing locally right now, that doesn't help to write tests that passes |
The current errors are due to the use of Otherwise, I would just copy the If you're having trouble running tests, feel free to post what issues you're running into, I may be able to help. |
Yes I figured it out but didn't fixed it yet.
Not sure you can help, here's my issues anyway:
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... |
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. |
I use rust stable 1.44.0 via rustup |
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 |
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 |
Is it OK now ? |
Looks great, thanks! I pushed a small commit to update the man page. |
📌 Commit a0fb62f has been approved by |
☀️ Test successful - checks-azure |
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)
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)
Fixes #8318