rust-lang / git2-rs

libgit2 bindings for Rust
https://docs.rs/git2
Apache License 2.0
1.64k stars 380 forks source link

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

Closed amyspark closed 10 months ago

amyspark commented 10 months ago

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 .

ehuss commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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

ehuss commented 10 months ago

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 commented 10 months ago

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!