Reintegrate hyper #509

brson · 2016-06-02T15:54:58Z

@sfackler has updated native-tls to use new schannel bindings. Let's add support back for hyper, alongside curl, switchable at runtime I think.

This will involve updating the rustup-utils crate to have two different download functions, the existing one that uses curl, and the new one that uses hyper and native-tls. Look at the revert of 5fc6377 to figure out how to write hyper + native-tls.

If the RUSTUP_USE_HYPER env var is set then use hyper, otherwise use curl.

cc @seanmonstar

The text was updated successfully, but these errors were encountered:

inejge · 2016-06-09T21:28:17Z

I'll give it a try. I expect that most work will involve adding proxy support: curl enables it by setting the proxy URL in the environment, and I want to keep that ability with hyper.

sfackler · 2016-06-09T21:54:20Z

I made a crate to connect through SOCKS proxies with hyper: https://crates.io/crates/hyper-socks. I think there was some work to add HTTP proxy support directly into hyper, but I'm not sure what the status of that is.

inejge · 2016-06-10T09:29:38Z

I think there was some work to add HTTP proxy support directly into hyper, but I'm not sure what the status of that is.

It works, but there's no built-in support for rust-native-tls, which rustup has to use. I'll see if it's possible to manually create Client instances using a proxy with a custom TLS wrapper.

sfackler · 2016-06-10T09:33:48Z

You should be able to grab that from the original implementation in this crate.

inejge · 2016-06-10T10:27:16Z

You should be able to grab that from the original implementation in this crate.

Right, but I can't use it with hyper's proxy support because hyper::client::proxy is private. I'll try grafting proxy.rs onto my own code, it's not too large and it seems reasonably self-contained.

sfackler · 2016-06-10T10:41:04Z

@seanmonster ^ appears to be an API hole in Client constructors.

inejge · 2016-06-10T12:45:44Z

It's ^ @seanmonstar (not a difficult typo to make 😄) Also, no dice with reimplementing proxy.rs locally, since Client.proxy is also private. I kludged my locally cached hyper source to make that field public, and managed to make hyper+rust-native-tls proxied connections work.

I'll think about what would be a minimal API change to allow a custom TLS wrapper for proxied connections.

seanmonstar · 2016-06-12T17:59:13Z

@sfackler is native-tls stable enough that I could add a 'tls' feature to hyper that uses it? If I did, would rustup need anything else custom, or could it just use the default connector and proxy?

(I'm not in love with the proxy api, and I'd rather focus energy improving it in async hyper.)

sfackler · 2016-06-13T15:46:22Z

@seanmonstar it's not quite ready yet - in particular it depends on a fork of winapi until some additions land.

Restore support for hyper + rust-native-tls, selectable at runtime by setting RUSTUP_USE_HYPER in the environment. Proxied connections are supported as well. Fixes rust-lang#509

inejge · 2016-06-14T19:10:31Z

Hyper 0.9.8 is now out with proxy TLS wrapper support ( @seanmonstar : thanks! ), so I've published a PR which incorporates the changes required in this issue.

brson · 2016-06-15T06:40:06Z

@inejge Yay! Thank you!

inejge · 2016-06-15T10:11:57Z

@sfackler I see that the Windows build is failing because schannel can't find constants in the published winapi. I slapped together a Windows build environment and managed to pass the schannel hurdle, only for the build to fail with:

src\rustup-utils\src\raw.rs:227:51: 227:63 error: the trait bound `*mut std::os::raw::c_void: std::marker::Send` is not satisfied [E0277]
src\rustup-utils\src\raw.rs:227             impl<T: NetworkStream + Send + Clone> SslClient<T> for NativeSslClient {
                                                                                  ^~~~~~~~~~~~
src\rustup-utils\src\raw.rs:227:51: 227:63 help: run `rustc --explain E0277` to see a detailed explanation
src\rustup-utils\src\raw.rs:227:51: 227:63 note: `*mut std::os::raw::c_void` cannot be sent between threads safely
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `schannel::cert_store::CertStore`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `std::option::Option<schannel::cert_store::CertStore>`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `schannel::TlsStream<T>`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `native_tls::imp::TlsStream<T>`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `native_tls::TlsStream<T>`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required because it appears within the type `raw::hyper::download_file::NativeSslStream<T>`
src\rustup-utils\src\raw.rs:227:51: 227:63 note: required by `hyper::net::SslClient`
src\rustup-utils\src\raw.rs:259:21: 259:34 error: the trait bound `*mut std::os::raw::c_void: std::marker::Send` is not satisfied [E0277]
src\rustup-utils\src\raw.rs:259             impl<T> NetworkStream for NativeSslStream<T>
                                                    ^~~~~~~~~~~~~
src\rustup-utils\src\raw.rs:259:21: 259:34 help: run `rustc --explain E0277` to see a detailed explanation
src\rustup-utils\src\raw.rs:259:21: 259:34 note: `*mut std::os::raw::c_void` cannot be sent between threads safely
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `schannel::cert_store::CertStore`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `std::option::Option<schannel::cert_store::CertStore>`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `schannel::TlsStream<T>`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `native_tls::imp::TlsStream<T>`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `native_tls::TlsStream<T>`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required because it appears within the type `raw::hyper::download_file::NativeSslStream<T>`
src\rustup-utils\src\raw.rs:259:21: 259:34 note: required by `hyper::net::NetworkStream`
error: aborting due to 2 previous errors
error: Could not compile `rustup-utils`.

My understanding of schannel is nil, so I don't know how to proceed from here.

sfackler · 2016-06-15T16:21:05Z

@inejge for the winapi issue, schannel depends on a fork of winapi until this PR merges: retep998/winapi-rs#288. You can use cargo's new top-level-override feature to force it to use my git fork until then: https://github.com/sfackler/rust-native-tls/blob/master/Cargo.toml#L16-L17.

For the Send and Sync things, I just need to add some unsafe impls - should be an easy fix when I get home tonight.

inejge · 2016-06-16T12:56:52Z

You can use cargo's new top-level-override feature to force it to use my git fork

@sfackler yes, I've noticed the override.

For the Send and Sync things, I just need to add some unsafe impls

Got it, thanks, it's

unsafe impl Send for CertStore { }

I've forked native-tls and schannel, and created a rustup branch which now builds cleanly on Windows (and works!) I've also submitted a PR to schannel.

inejge · 2016-06-16T21:13:27Z

@brson I pushed another commit to #532, which should have produced clean builds, however both failed.

Travis couldn't compile any Linux version (Darwin succeeded). The logs say:

+ cargo build --release --target x86_64-unknown-linux-gnu
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/Diggsey/markdown.rs.git`
    Updating git repository `https://github.com/sfackler/winapi-rs`
    Updating git repository `https://github.com/sfackler/rust-native-tls.git`
    Updating git repository `https://github.com/sfackler/schannel-rs`

error: failed to write /buildslave/Cargo.lock

Caused by:
  failed to open: /buildslave/Cargo.lock

To learn more, run the command again with --verbose.

This looks like a build environment problem. FWIW, OpenSSL was successfully compiled before this failure in the same run.

The Windows build failed because, for some reason, Cargo fetched an older commit from the "rewrite" branch of schannel (f4ee623 instead of fa053fe.)

inejge · 2016-06-17T08:03:50Z

The Windows build failed because, for some reason, Cargo fetched an older commit from the "rewrite" branch of schannel

"Some reason" being the commit hash written in Cargo.lock. Duh. Another commit inoming.

Fixes rust-lang#509

inejge · 2016-06-17T10:18:10Z

Linux builds are still failing.
OS X builds are passing.
3 out of 4 Windows builds passed, the last failed during preparation:

set PATH=%PATH%;C:\Users\appveyor\.cargo\bin
rustup default nightly-2016-05-10-x86_64-pc-windows-msvc
info: syncing channel updates for 'nightly-2016-05-10-x86_64-pc-windows-msvc' 
info: downloading component 'rustc' 
error: component download failed for rustc-x86_64-pc-windows-msvc
info: caused by: could not download file from 'https://static.rust-lang.org/dist/2016-05-10/rustc-nightly-x86_64-pc-windows-msvc.tar.gz' to 'C:\Users\appveyor\.multirust\tmp\eyy8jb26wi7kgy4x_file
info: caused by: http request returned failure: [18] Transferred a partial file
Command exited with code 1

Update Cargo.lock so that CI builders don't try to modify it, as it is read-only during the build. Fixes rust-lang#509

inejge · 2016-06-18T19:57:31Z

All builds passing now.

Restore support for hyper + rust-native-tls, selectable at runtime by setting RUSTUP_USE_HYPER in the environment. Proxied connections are supported as well. Fixes rust-lang#509

Fixes rust-lang#509

Update Cargo.lock so that CI builders don't try to modify it, as it is read-only during the build. Fixes rust-lang#509

brson added the help wanted label Jun 2, 2016

inejge mentioned this issue Jun 12, 2016

No way to use a custom TLS wrapper for proxy connections hyperium/hyper#824

Closed

inejge mentioned this issue Jun 14, 2016

Bring back hyper with proxy support #532

Merged

inejge mentioned this issue Jun 16, 2016

Add Send impl for CertStore to make it usable in native-tls and rustup sfackler/schannel-rs#10

Merged

inejge added a commit to inejge/rustup.rs that referenced this issue Jun 17, 2016

Further fix to use correct schannel commit

9b74d85

Fixes rust-lang#509

inejge added a commit to inejge/rustup.rs that referenced this issue Jun 17, 2016

Fix to make a CI build pass

6ca277d

Update Cargo.lock so that CI builders don't try to modify it, as it is read-only during the build. Fixes rust-lang#509

brson closed this as completed in #532 Jun 20, 2016

nodakai pushed a commit to nodakai/rustup.rs that referenced this issue Apr 23, 2017

Further fix to use correct schannel commit

bc3bd6f

Fixes rust-lang#509

nodakai pushed a commit to nodakai/rustup.rs that referenced this issue Apr 23, 2017

Fix to make a CI build pass

eb70e3b

Update Cargo.lock so that CI builders don't try to modify it, as it is read-only during the build. Fixes rust-lang#509

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintegrate hyper #509

Reintegrate hyper #509

brson commented Jun 2, 2016

inejge commented Jun 9, 2016

sfackler commented Jun 9, 2016

inejge commented Jun 10, 2016

sfackler commented Jun 10, 2016

inejge commented Jun 10, 2016

sfackler commented Jun 10, 2016

inejge commented Jun 10, 2016

seanmonstar commented Jun 12, 2016

sfackler commented Jun 13, 2016

inejge commented Jun 14, 2016

brson commented Jun 15, 2016

inejge commented Jun 15, 2016

sfackler commented Jun 15, 2016

inejge commented Jun 16, 2016

inejge commented Jun 16, 2016

inejge commented Jun 17, 2016

inejge commented Jun 17, 2016

inejge commented Jun 18, 2016

Reintegrate hyper #509

Reintegrate hyper #509

Comments

brson commented Jun 2, 2016

inejge commented Jun 9, 2016

sfackler commented Jun 9, 2016

inejge commented Jun 10, 2016

sfackler commented Jun 10, 2016

inejge commented Jun 10, 2016

sfackler commented Jun 10, 2016

inejge commented Jun 10, 2016

seanmonstar commented Jun 12, 2016

sfackler commented Jun 13, 2016

inejge commented Jun 14, 2016

brson commented Jun 15, 2016

inejge commented Jun 15, 2016

sfackler commented Jun 15, 2016

inejge commented Jun 16, 2016

inejge commented Jun 16, 2016

inejge commented Jun 17, 2016

inejge commented Jun 17, 2016

inejge commented Jun 18, 2016