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

Add socks5 support for reqwest-backend #2466

Merged
merged 3 commits into from
Sep 3, 2020
Merged

Add socks5 support for reqwest-backend #2466

merged 3 commits into from
Sep 3, 2020

Conversation

eidenar
Copy link
Contributor

@eidenar eidenar commented Aug 20, 2020

As discussed in #2409, rustup doesn't use socks5 proxy with reqwest backend. It turned out, that enabling socks feature in Cargo.toml is enough to make it work as intended.

There was no updates from the last person worked on this issue for almost a month, so I decided to pick it up.

@kinnison
Copy link
Contributor

I wonder if there's any way we can add a test to ensure that we don't accidentally lose this capability?

@eidenar
Copy link
Contributor Author

eidenar commented Aug 24, 2020

@kinnison I added a test to check if socks5 feature is enabled. It performs a simple request with non-existent socks5 address from all_proxy variable and checks call count. Not sure thought if there is a easier way. But I ran this test without socks feature and it failed as expected.

@eidenar eidenar requested a review from kinnison August 24, 2020 21:02
@kinnison
Copy link
Contributor

I've rebased this (as Cargo.lock was in conflict)

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

At this point I'm satisfied with the shape of this, but @eidenar needs to double-check and then rebase the series so that we have a nice clean commit series of the change and the test, without the corrections.

Eduard Miller and others added 3 commits August 31, 2020 15:37
In order to ensure that we do not stomp on each other when running
tests in the download crate to verify proxy behaviour, introduce a
Mutex to guard against this.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@eidenar
Copy link
Contributor Author

eidenar commented Aug 31, 2020

I've updated commits and messages to be clear, @kinnison.

@eidenar eidenar requested a review from kinnison August 31, 2020 16:18
Copy link
Contributor

@kinnison kinnison 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 good to me, thank you for your contribution.

@kinnison kinnison merged commit 2869670 into rust-lang:master Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants