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

Fix vendored-openssl feature not overriding libssh2-sys too #983

Closed

Conversation

amyspark
Copy link

Hi!

This is a MR to fix how vendored-openssl overrides openssl-sys. Currently, libssh2-sys escapes this override and, at least on Darwin with brewed OpenSSL, this causes a linker failure due to it bringing a different version of OpenSSL.

cc @nirbheek at GStreamer, where he overrode it with the environment variables: https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/1258 .

This causes a linker failure on macOS due to libssh2-sys still bringing
Homebrewed OpenSSL 3.
@ehuss
Copy link
Contributor

ehuss commented Aug 26, 2023

Thanks for the PR! Unfortunately I'm not sure I understand what the issue is here. Can you open an issue with a reproduction?

When libgit2-sys enables the vendored-openssl feature, it enables it in openssl-sys/vendored. Due to feature unification, there is only one copy of openssl-sys, and thus the one used by libssh2-sys should also get the vendored feature.

Also, this change wouldn't be correct because libssh2-sys is an optional dependency, but this change would force it to be enabled without the ssh feature. I believe this would require the ? syntax to avoid that.

@amyspark
Copy link
Author

@ehuss hi! to reproduce it you'd need to install GStreamer's Cerbero from here: https://gitlab.freedesktop.org/amyspark/cerbero/-/tree/fix-rust-linking (sending you the full branch I am working on).

Change into the working directory in a Bash shell with Homebrew visible, install openssl@1 and openssl, and run:

./cerbero-uninstalled -c ./config/cross-ios-universal.cbc -v rust bootstrap

It'll take a while, but once it reaches cargo-c, it will fail at the very last step with:

 = note: Undefined symbols for architecture x86_64:
            "_EVP_PKEY_get_id", referenced from:
                __libssh2_ed25519_new_private_frommemory in liblibssh2_sys-924099457469ec8b.rlib(openssl.o)
                __libssh2_pub_priv_keyfile in liblibssh2_sys-924099457469ec8b.rlib(openssl.o)
                __libssh2_pub_priv_keyfilememory in liblibssh2_sys-924099457469ec8b.rlib(openssl.o)
          ld: symbol(s) not found for architecture x86_64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

The log will show that starting with libssh2-sys, it'll pick up Homebrew's OpenSSL 3. (Note that Cerbero does not allow picking external dependencies; in this case, the crate does not follow pkg-config and forcefully reaches to brew before trying any other lookup method.)

@amyspark
Copy link
Author

amyspark commented Sep 6, 2023

Hi, is there anything else I can help with to move this PR forward?

@ehuss
Copy link
Contributor

ehuss commented Sep 6, 2023

Yes, can you please open an issue with a reproduction that is as isolated as possible? For example, show a cargo manifest like:

[package]
name = "foo"
version = "0.1.0"

[dependencies]
git2 = "0.18.0"

and the command to run, like:

cargo build -F vendored-openssl

that illustrates the problem. Please also include full information, like which operating system you are using, the output of cargo --version --verbose, anything that needs to be installed on the system (you mentioned homebrew, please give the exact information of what needs to be installed, like which openssl needs to be installed). I would prefer to not have a reproduction that uses another build system.

And please provide some information why you think this change would fix the problem? I do not see why this PR would be necessary. Due to cargo feature unification, the vendored featured is set on openssl-sys, and both ssh2-rs and git2-rs share the same version. I don't see how ssh2-sys would change with this feature set.

@amyspark
Copy link
Author

I couldn't craft a matching test crate for you. I think this may be due to rustflags force prepending the OpenSSL 1.x lookup directory. Closing for now, apologies!

@amyspark amyspark closed this Sep 13, 2023
@amyspark amyspark deleted the fix-incomplete-vendored-openssl branch September 13, 2023 17:38
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.

2 participants