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 library lookup path in the vendored OpenSSL #2493

Merged
merged 2 commits into from
Jan 11, 2025
Merged

Conversation

mislav
Copy link
Member

@mislav mislav commented Jan 10, 2025

When a vendored OpenSSL is needed for compiling Ruby, that OpenSSL installation ends up with its bin/openssl executable broken due to not finding "libssl.so" and "libcrypto.so" in the global load path for libraries. This doesn't seem to negatively affect the Ruby "openssl" extension at runtime, but does constitute a broken OpenSSL installation nevertheless.

This change causes the bin/openssl executable and related shared libraries to be built with an "RPATH" pointing to the "lib" directory of the vendored OpenSSL. See https://wiki.openssl.org/index.php/Compilation_and_Installation#Using_RPATHs

Fixes #2488

When a vendored OpenSSL is needed for compiling Ruby, that OpenSSL installation
ends up with its `bin/openssl` executable broken due to not finding "libssl.so"
and "libcrypto.so" in the global load path for libraries. This doesn't seem to
negatively affect the Ruby "openssl" extension, but is a broken OpenSSL install
nevertheless.

This change causes the `bin/openssl` executable and related shared libraries to
be built with an "RPATH" pointing to the "lib" directory of the vendored OpenSSL.
@mislav mislav requested a review from eregon January 10, 2025 17:31
@eregon
Copy link
Member

eregon commented Jan 11, 2025

I checked locally and it works, thank you for the quick fix!

@eregon eregon merged commit 068c404 into master Jan 11, 2025
6 checks passed
@eregon eregon deleted the openssl-rpath branch January 11, 2025 14:15
@@ -1215,7 +1215,7 @@ build_package_openssl() {
[[ "$1" != openssl-1.0.* ]] || nokerberos=1

# Compile a shared lib with zlib dynamically linked.
package_option openssl configure --openssldir="$OPENSSLDIR" --libdir="lib" zlib-dynamic no-ssl3 shared ${nokerberos:+no-ssl2 no-krb5}
package_option openssl configure --openssldir="$OPENSSLDIR" --libdir="lib" -Wl,-rpath="$OPENSSL_PREFIX_PATH/lib" zlib-dynamic no-ssl3 shared ${nokerberos:+no-ssl2 no-krb5}
Copy link
Member

Choose a reason for hiding this comment

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

clang -fPIC -arch arm64 -O3 -Wall -L. -dynamiclib -current_version 1.1 -compatibility_version 1.1 -Wl,-search_paths_first -L/opt/homebrew/opt/ruby/lib -Wl,-rpath=/Users/dorin/.local/share/mise/installs/ruby/2.7.8/openssl/lib -install_name /Users/dorin/.local/share/mise/installs/ruby/2.7.8/openssl/lib/libcrypto.1.1.dylib -o …
                  
ld: unknown options: -rpath=/Users/dorin/.local/share/mise/installs/ruby/2.7.8/openssl/lib 
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Looks like this needs to use -Wl,-rpath,"$OPENSSL_PREFIX_PATH/lib" for Clang (comma rather than equals).

Copy link
Member

Choose a reason for hiding this comment

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

RPATH is automatically set up on BSDs, so this is only needed on Linux. Also, comma syntax appears to be preferred.

Suggested change
package_option openssl configure --openssldir="$OPENSSLDIR" --libdir="lib" -Wl,-rpath="$OPENSSL_PREFIX_PATH/lib" zlib-dynamic no-ssl3 shared ${nokerberos:+no-ssl2 no-krb5}
# Add RPATH for Linux/GCC
case "$(uname -s)" in
Linux)
if cc -v 2>&1 | grep -q "gcc"; then
EXTRA_RPATH_FLAGS="-Wl,-rpath,${OPENSSL_PREFIX_PATH}/lib -Wl,--enable-new-dtags"
fi
;;
esac
package_option openssl configure --openssldir="$OPENSSLDIR" --libdir="lib" zlib-dynamic no-ssl3 shared ${nokerberos:+no-ssl2 no-krb5}

Copy link
Member

@eregon eregon Jan 14, 2025

Choose a reason for hiding this comment

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

Could you make a PR?
BTW clang on Linux would also need the RPATH, so we shouldn't do this only with gcc.
Re --enable-new-dtags I don't know if necessary, I got a RUNPATH anyway without it, it seems recent Linux changed the default anyway.
(BTW I wonder why OpenSSL only sets up RPATH automatically on BSD (according to docs), where it's probably more complicated even)

Copy link
Member

Choose a reason for hiding this comment

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

Can't shepherd through a PR or test on all these platforms at the moment. Recommend reverting this PR for now so macOS users can install Ruby.

Note that --enable-new-dtags is set by default on some distros but not others. If you have a RUNPATH, it's because it's on by default for you (Gentoo, Debian, … others?). Behavior is subtly different with RUNPATH taking precedence and respecting LD_LIBRARY_PATH thereafter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jeremy. I've undone rpath argument passing on non-Linux platforms, and added clang detection.

I've also expedited a release thinking that the previous change already made it into a release, but it turns out I've just released both changes together, so fingers crossed that this is the right incantation. https://github.com/rbenv/ruby-build/releases/tag/v20250114

If more regressions are discovered, I think I would also revert the whole rpath thing since it's more trouble than it's worth. I must admit I don't know much about RPATH vs RUNPATH, but openssl should not need this many extra compiler flags just to produce a working build in a custom location.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Looks like mise installs ruby-build from git which gave us an early preview of the forthcoming release. Nice to catch this early, then.

@eregon
Copy link
Member

eregon commented Jan 15, 2025

I filed two openssl issues upstream for the gotchas mentioned here: openssl/openssl#26422 openssl/openssl#26423

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.

The openssl built by ruby-build has somewhat broken library dependencies
3 participants