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

Update to 8.10.1 #573

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Update to 8.10.1 #573

merged 3 commits into from
Sep 20, 2024

Conversation

ehuss
Copy link
Collaborator

@ehuss ehuss commented Sep 18, 2024

@alexcrichton
Copy link
Owner

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 *.bat file presumably thinking the host is windows).

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?

@ehuss
Copy link
Collaborator Author

ehuss commented Sep 18, 2024

If I am understanding the changes in rustls correctly, it looks like they switched the default from ring to aws-lc-rs. I'm not sure what the motivation is behind that. Another option is to switch back to ring:

--- 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?

@alexcrichton
Copy link
Owner

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

@ehuss
Copy link
Collaborator Author

ehuss commented Sep 19, 2024

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 default-features=false + features=["ring"].

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

@alexcrichton
Copy link
Owner

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.

@ehuss
Copy link
Collaborator Author

ehuss commented Sep 20, 2024

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?

@alexcrichton
Copy link
Owner

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?

@glandium
Copy link
Contributor

it's trying to execute a *.bat file presumably thinking the host is windows

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.
@ehuss
Copy link
Collaborator Author

ehuss commented Sep 20, 2024

Sounds good. I also dropped i686-pc-windows-msvc. I can re-add x86_64-pc-windows-gnu whenever it gets fixed.

@alexcrichton alexcrichton merged commit a09b66e into alexcrichton:main Sep 20, 2024
13 checks passed
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.

Please update to curl 8.10.1
3 participants