Add LIBSSH2_SYS_USE_PKG_CONFIG env var

[Timmmm]

The latest actual release of libssh2 is old and broken. This uses the submodule instead unless you set LIBSSH2_SYS_USE_PKG_CONFIG. The mechanism is copied from libgit2.

I haven't tested this, because I don't know how to get cargo to build everything with these changes. :-/

[alexcrichton]

I think this is for #87, right? If so, can you also add a comment or a note here as to why the latest release is broken? I forget at this point :(

[Timmmm]

It was this issue. The latest release (1.8.0) isn't actually as old as I thought, though their releases do seem to be fairly infrequent so we might be waiting a while for another release.

Even when they do do another release with that fix, I think it is good to have an option to use the vendored code, otherwise it's hard to make dependency-free binaries. Also if you randomly compile against system libraries it is very difficult to get hermetic / reproducible builds.

[alexcrichton]

Hm is it know why the system library failed there? For example where the segfault was coming from?

[Timmmm]

No idea. Here is where it fails for me:

frame #1: 0x000000010045435c libssh2.1.dylib`kex_method_diffie_hellman_group_exchange_sha256_key_exchange + 3367
libssh2.1.dylib`kex_method_diffie_hellman_group_exchange_sha256_key_exchange:
    0x10045435c <+3367>: leaq   0x14fac(%rip), %rsi       ; "SSH-2.0-libssh2_1.8.0"
    0x100454363 <+3374>: movl   $0x15, %edx
    0x100454368 <+3379>: movq   %rbx, %rdi
    0x10045436b <+3382>: callq  0x100467b56               ; symbol stub for: EVP_DigestUpdate

Which seems to be this code, but I have no idea why that would fail and it hasn't been changed recently. And I can't work out what the hell that leaq instruction is trying to do. x86 assembly is not fun.

[alexcrichton]

Can you tell at runtime if there's perhaps two OpenSSL libraries loaded? Otherwise do you know what the faulting instruction is and can it perhaps be gleaned why it's the faulting instruction?

[Timmmm]

Ah yeah that could be it:

(lldb) image list
...
[  6] 93344C6C-82DA-3753-BFC4-E9937C66F123 0x000000010044c000 /usr/local/opt/libssh2/lib/libssh2.1.dylib 
[  7] 9F02EB03-7801-350F-B8EF-6FE3C63262D5 0x0000000100473000 /usr/local/opt/[email protected]/lib/libssl.1.1.dylib 
[  8] 5DA76CDA-D74C-3C5A-9CAC-D37986F93269 0x00000001004d4000 /usr/local/opt/[email protected]/lib/libcrypto.1.1.dylib 
...
[146] BB4F0EC6-7469-3244-9366-3848B585702A 0x00000001006ef000 /usr/local/opt/openssl/lib/libssl.1.0.0.dylib 
[147] 1FD000A9-05A4-3F7D-BB0C-AA34D5DC0235 0x000000010074d000 /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib 

Well that sucks.

[alexcrichton]

Hm ok that'd definitely cause problems! Mind leaving that in a comment (and maybe a link to this PR?)? After that should be good to merge!

[alexcrichton]

Thanks!

[infinity0]

In general can we please avoid these random magic environment variables? If the version is broken can you just detect it automatically? These random magic environment variables make things awkward for packagers, since it makes things not work out-of-the-box and we have to figure out which envvars to set to get things working again.

[infinity0]

If the version is broken can you just detect it automatically?

I mean, by detecting the actual thing that is broken, rather than a version number, because we can and do patch things in Debian to fix broken versions.

[infinity0]

For context: in Debian we are forced to patch out this PR, because it is easier than trying to set the environment variable in all crates that depend on this crate. Debian packaging for Rust crates are automatically generated, so there is no easy way to detect whether any (direct or indirect) dependency requires some magic environment variable in order to "just work" with system libs. So we cannot really set this environment variable in all the cases where it's needed, it's easier to just patch this out.

[Timmmm]

Fair point. I wonder if cargo needs a general option to tell packages whether or not to use system dependencies where possible.

I can see that in Debian you basically always want to depend on other packages, but if you're building a binary that you are distributing outside of a package manager you basically never want to use system libraries.

Perhaps another solution for Debian would be to create a central script that all Rust packages run, then you can just add this environment variable to that script if you really do want it for all packages the use this create.

The ability to make easy to distribute binaries without having to worry about system dependencies is one of the things people absolutely love about Go and I don't think Rust should give it up lightly.

[infinity0]

We do have a central script that all Rust packages run called dh-cargo but it's not very sustainable to have to update it every time some rust crate decides they need their own environment variable.

If the Rust ecosystem as a whole could decide on one single environment variable to use for this purpose (e.g. RUST_ALL_CRATES_USE_PKG_CONFIG) then it would make life much easier and yes we can definitely add this to the central script.