1369665 - "mach vendor rust" command has to export the OpenSSL lib folder

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now linking of cargo-vendor will fail because we do not export the openssl lib folder, but only include, and that twice:

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/python/mozbuild/mozbuild/vendor_rust.py#86-93

What's missing here is something like:

OPENSSL_LIB_DIR=`brew --prefix openssl`/lib

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So for Servo they took a similar change:
https://github.com/servo/servo/pull/13225

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1369665 - "mach vendor rust" has to export the OpenSSL lib folder.

It's not enough to only export the include folder for openssl, but for
linking the lib folder is also necessary. Also DEP_OPENSSL_INCLUDE
shouldn't be set directly, because it's set by Cargo itself based
on the output of openssl-sys's build script.

Review commit: https://reviewboard.mozilla.org/r/145228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145228/

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8873799 [details]
Bug 1369665 - "mach vendor rust" has to export the OpenSSL lib folder.

https://reviewboard.mozilla.org/r/145228/#review149282

Thanks for the patch! This is definitely and improvement, so I'll go ahead and merge. What system did you get the link failure on? On mine (macOS 10.12.5 + homebrew) `cargo install cargo-vendor` works fine, and ends up linked to the correct libssl.dylib. It looks like there's code in recent releases of openssl-sys to find the homebrew openssl install, so I'm wondering if we can just remove these variables entirely.

::: commit-message-aeb3d:6
(Diff revision 1)
> +Bug 1369665 - "mach vendor rust" has to export the OpenSSL lib folder.
> +
> +It's not enough to only export the include folder for openssl, but for
> +linking the lib folder is also necessary. Also DEP_OPENSSL_INCLUDE
> +shouldn't be set directly, because it's set by Cargo itself based
> +on the output of openssl-sys's build script.

Can you explain how this happens? The [docs](http://doc.crates.io/build-script.html#the-links-manifest-key) suggest the `DEP_FOO` variables are explicitly set by build.rs, as a way to communicate ad-hoc metadata.

I didn't find any code in cargo for generating something like this for C-language include paths.

The point may be moot now though. I don't remember why I added this line, but nothing in the current openssl crate uses it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d62f2d63e674
"mach vendor rust" has to export the OpenSSL lib folder. r=rillian

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d62f2d63e674

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873799 [details]
Bug 1369665 - "mach vendor rust" has to export the OpenSSL lib folder.

https://reviewboard.mozilla.org/r/145228/#review149282

I also have MacOS 10.12.5 and Homebrew 1.2.1 installed. Here the output from `brew list openssl`:

% brew list openssl
/usr/local/Cellar/openssl/1.0.2k/bin/c_rehash
/usr/local/Cellar/openssl/1.0.2k/bin/openssl
/usr/local/Cellar/openssl/1.0.2k/include/openssl/ (75 files)
/usr/local/Cellar/openssl/1.0.2k/lib/libcrypto.1.0.0.dylib
/usr/local/Cellar/openssl/1.0.2k/lib/libssl.1.0.0.dylib
/usr/local/Cellar/openssl/1.0.2k/lib/engines/ (12 files)
/usr/local/Cellar/openssl/1.0.2k/lib/pkgconfig/ (3 files)
/usr/local/Cellar/openssl/1.0.2k/lib/ (4 other files)
/usr/local/Cellar/openssl/1.0.2k/share/man/ (1592 files)

So not sure which version you have, and if you have those env variables already globally setup?

> Can you explain how this happens? The [docs](http://doc.crates.io/build-script.html#the-links-manifest-key) suggest the `DEP_FOO` variables are explicitly set by build.rs, as a way to communicate ad-hoc metadata.
> 
> I didn't find any code in cargo for generating something like this for C-language include paths.
> 
> The point may be moot now though. I don't remember why I added this line, but nothing in the current openssl crate uses it.

Sorry, I just copied what several people mentioned across different github related issues for Cargo. And given that the DEP variable didn't help, I trusted that.