-
Notifications
You must be signed in to change notification settings - Fork 238
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
Update to 8.10.1 #573
Update to 8.10.1 #573
Conversation
76c3081
to
43d7bc8
Compare
Hm ok the latest error looks a bit worrisome. The rustls-ffi update is pulling in a new dependency on cmake which is easily resolved but it looks like it's not equipped to be cross-compiled to mingw from linux (it's trying to execute a Is there perhaps a flag or feature we should pass in CI to avoid building aws-lc or an alternative backend when compiling for mingw? |
If I am understanding the changes in rustls correctly, it looks like they switched the default from --- a/curl-sys/Cargo.toml
+++ b/curl-sys/Cargo.toml
@@ -24,7 +24,8 @@ libnghttp2-sys = { optional = true, version = "0.1.3" }
[dependencies.rustls-ffi]
version = "0.14"
optional = true
-features = ["no_log_capture"]
+features = ["no_log_capture", "ring"]
+default-features = false
[target.'cfg(all(unix, not(target_os = "macos")))'.dependencies]
openssl-sys = { version = "0.9.64", optional = true } Or, if you think it would be better to only do that for mingw (or windows in general?), then we could maybe do that with some features shenanigans. WDYT? |
Perhaps only updating CI? I'm not sure if it's easy to configure just the mingw builder to do something differently but I think it'd be best if we didn't force one option or another as a dependency through curl |
It's a bit tricky to switch the backend. With rustls's default features, it always uses aws_lc_rs. With no default features, it's just broken (there's no crypto). So to use ring, it needs to be So that approach might look something like: diff --git a/Cargo.toml b/Cargo.toml
index ed871cf..7c3b396 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -41,6 +41,7 @@ mesalink = ["curl-sys/mesalink"] # MesaLink TLS backend
http2 = ["curl-sys/http2"]
spnego = ["curl-sys/spnego"]
rustls = ["curl-sys/rustls"]
+rustls-ring = ["curl-sys/rustls-ring"]
static-curl = ["curl-sys/static-curl"]
static-ssl = ["curl-sys/static-ssl"]
windows-static-ssl = ["static-curl", "curl-sys/windows-static-ssl"]
diff --git a/ci/run.sh b/ci/run.sh
index 6c2dae3..d71fef8 100755
--- a/ci/run.sh
+++ b/ci/run.sh
@@ -6,7 +6,12 @@ cargo test --target $TARGET --no-run
# First test with no extra protocols enabled.
cargo test --target $TARGET --no-run --features static-curl
# Then with rustls TLS backend.
-cargo test --target $TARGET --no-run --features rustls,static-curl
+if [ "$TARGET" = "x86_64-pc-windows-gnu" ]
+then
+ cargo test --target $TARGET --no-run --features rustls-ring,static-curl
+else
+ cargo test --target $TARGET --no-run --features rustls,static-curl
+fi
# Then with all extra protocols enabled.
cargo test --target $TARGET --no-run --features static-curl,protocol-ftp,ntlm
if [ -z "$NO_RUN" ]; then
diff --git a/curl-sys/Cargo.toml b/curl-sys/Cargo.toml
index 51e3002..db679c1 100644
--- a/curl-sys/Cargo.toml
+++ b/curl-sys/Cargo.toml
@@ -25,6 +25,7 @@ libnghttp2-sys = { optional = true, version = "0.1.3" }
version = "0.14"
optional = true
features = ["no_log_capture"]
+default-features = false
[target.'cfg(all(unix, not(target_os = "macos")))'.dependencies]
openssl-sys = { version = "0.9.64", optional = true }
@@ -44,7 +45,8 @@ default = ["ssl"]
ssl = ["openssl-sys"]
http2 = ["libnghttp2-sys"]
mesalink = []
-rustls = ["rustls-ffi"]
+rustls = ["dep:rustls-ffi", "rustls-ffi/aws-lc-rs"]
+rustls-ring = ["dep:rustls-ffi", "rustls-ffi/ring"]
static-curl = []
windows-static-ssl = []
static-ssl = ["openssl-sys/vendored"] which seems to work. An alternate approach is to just disable the rustls test for the mingw cross compile, which might look something like: diff --git a/ci/run.sh b/ci/run.sh
index 6c2dae3..ff03e28 100755
--- a/ci/run.sh
+++ b/ci/run.sh
@@ -6,7 +6,12 @@ cargo test --target $TARGET --no-run
# First test with no extra protocols enabled.
cargo test --target $TARGET --no-run --features static-curl
# Then with rustls TLS backend.
-cargo test --target $TARGET --no-run --features rustls,static-curl
+# Note: Cross-compiling rustls for windows doesn't work due to requiring some
+# NASM build stuff in aws_lc_rs.
+if [ "$TARGET" != "x86_64-pc-windows-gnu" ]
+then
+ cargo test --target $TARGET --no-run --features rustls,static-curl
+fi
# Then with all extra protocols enabled.
cargo test --target $TARGET --no-run --features static-curl,protocol-ftp,ntlm
if [ -z "$NO_RUN" ]; then |
Thanks for investigating! Given that I'd personally lean towards just removing the rustls test on mingw since it seems like we're otherwise swimming upstream against the aws-lc crate and rustls configuration which seems not great. |
995f1c0
to
23c3d83
Compare
So I see aws/aws-lc-rs#528 is opened to potentially fix this. Perhaps we should just wait and see if that lands soon? |
Given that we're just frobbing the CI here in this repo I think it's fine to go ahead and land this when ready, we can back out the CI bits once aws-lc has had a release. Given the failure on CI though it may be the case that aws-lc doesn't support i686? Perhaps another platform to exclude rustls from in CI? |
It is supposed not to do that if nasm is installed on the system. So presumably, you should be able to install nasm on your CI and not have to care about forcing rustls. |
Rustls no longer works for cross-compiling due to switching to aws-lc (from ring). It seems to want to run `.bat` files for some NASM-related stuff. We could, in the future, expose `ring` as a rustls backend as a feature if someone wants to continue using that.
It requires nasm to be installed, and I don't want to hassle with that right now.
23c3d83
to
37ddb50
Compare
Sounds good. I also dropped i686-pc-windows-msvc. I can re-add x86_64-pc-windows-gnu whenever it gets fixed. |
Minor update: https://daniel.haxx.se/blog/2024/09/18/curl-8-10-1/
Closes #572