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.

Sign up for GitHub

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

Jump to bottom

Update vendored version to openssl 3 #1925

Merged
merged 1 commit into from Sep 4, 2023
Merged

Update vendored version to openssl 3 #1925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion openssl-sys/Cargo.toml
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ bssl-sys = { version = "0.1.0", optional = true }
[build-dependencies]
bindgen = { version = "0.64.0", optional = true, features = ["experimental"] }
cc = "1.0.61"
openssl-src = { version = "111", optional = true }
openssl-src = { version = "300.1.2", optional = true, features = ["legacy"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legacy feature is required for the tests at least.

Copy link
Owner

Choose a reason for hiding this comment

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

It does seem like we realistically probably want to enable this - thoughts @alex?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We enable it by default in pyca/cryptography, its required for basically any encrypted key serialization (since they all use garbage algorithms).

pkg-config = "0.3.9"
vcpkg = "0.2.8"

Expand Down
10 changes: 10 additions & 0 deletions openssl-sys/build/main.rs
View file
Open in desktop
Expand Up @@ -115,6 +115,16 @@ fn main() {
println!("cargo:rustc-link-lib={}={}", kind, lib);
}

// https://github.com/openssl/openssl/pull/15086
if version == Version::Openssl3xx
&& kind == "static"
&& (env::var("CARGO_CFG_TARGET_OS").unwrap() == "linux"
|| env::var("CARGO_CFG_TARGET_OS").unwrap() == "android")
&& env::var("CARGO_CFG_TARGET_POINTER_WIDTH").unwrap() == "32"
{
println!("cargo:rustc-link-lib=dylib=atomic");
Copy link
Contributor Author

@amousset amousset Jul 16, 2023 •
edited

Choose a reason for hiding this comment

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

When upgrading to 3.1, vendored tests on arm-unknown-linux-gnueabihf fail with undefined reference to __atomic_stuff link errors, fixed by re-applying the patch removed in 32a303a. I did not dig the problem deeper.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I guess I'll need to poke around a bit to see if the commit that fixed this was reverted or something.

This is also something that may actually be best off moved to openssl-src.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I think we just need to update the logic in openssl-src to include atomic as a lib when appropriate: https://github.com/alexcrichton/openssl-src-rs/blob/main/src/lib.rs#L549

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The libs to link don't seem to come from openssl-src (only the library path is used) but are redefined in openssl-sys. We could try to take lib list from openssl-src but it would still require defining them for non-vendored cases so I'm not sure it would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sfackler We're new ~ 1 week from 1.1 EOL, is the current PR acceptable or do you want additional changes? As stated in the previous comment it does not seem practical to include the atomic lib on the openssl-src side.

Copy link
Contributor

@BlackDex BlackDex Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

I Know this is merged already, but i wanted to provide my feedback.
This now broke armv6 arm-unknown-linux-gnueabi again, for which this was removed.
If this really is needed for vendored tests, then maybe only enable this when vendored is enabled, or, as i do this in my build scripts add it when needed like this RUSTFLAGS="-Clink-args=-latomic".

Copy link

Choose a reason for hiding this comment

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

The issue about the resulting regression is here: #2043
It broke my musl builds as well: deltachat/deltachat-core-rust#4799

}

if kind == "static" && target.contains("windows") {
println!("cargo:rustc-link-lib=dylib=gdi32");
println!("cargo:rustc-link-lib=dylib=user32");
Expand Down