-
-
Notifications
You must be signed in to change notification settings - Fork 13
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 static linking when system libraries installed #93
Conversation
Ruby's mkmf appears to list `RbConfig::CONFIG['exec_prefix']/lib` first in the search path when building a C extension. If you have libre2 installed in that path, the linker will use that library instead of the vendored library. This adds a test to ensure this case is covered.
Ruby's mkmf appears to list `RbConfig::CONFIG['exec_prefix']/lib` first in the search path when building a C extension. If you have libre2 installed in that path, the linker will use that library instead of the vendored library. This causes an undefined symbol error when using the extension. ``` /usr/local/bin/ruby: symbol lookup error: /usr/local/bundle/gems/re2-2.0.0.beta1/lib/re2.so: undefined symbol: _ZN3re23RE2C1ESt17basic_string_viewIcSt11char_traitsIcEERKNS0_7OptionsE ``` This commit does two things to ensure the final shared library is compiled statically with the vendored libraries: 1. For -L<path> flags, ensure that any `ports` paths are prioritized just in case there are installed libraries that might take precedence. 2. For -l<lib> flags, convert the library to the static library with a full path and substitute the absolute static library. For example, -lre2 maps to /path/to/ports/<arch>/libre2/<version>/lib/libre2.a.
Is any part of this worth upstreaming into MiniPortile2 or is it specific to our setup? |
Good question. I think this is outside the purview of MiniPortile2; Nokogiri does something similar. I suspect |
@@ -62,7 +63,9 @@ jobs: | |||
with: | |||
name: cruby-gem | |||
path: gems | |||
- run: ./scripts/test-gem-install gems --enable-system-libraries | |||
- name: Add a symlink for libre2 | |||
run: ln -s /usr/lib/libre2.so `ruby -e "puts RbConfig::CONFIG['exec_prefix']"`/lib/libre2.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is exec_prefix
on these runners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked, but according to https://github.com/mudge/re2/actions/runs/6126602337/job/16630989217?pr=93, I suspect it's something like /opt/hostedtoolcache/Ruby/3.1.4/x64
. ruby/setup-ruby#140 describes another way of retrieving the prefix.
@@ -128,7 +128,7 @@ def build_extension | |||
have_header("stdint.h") | |||
have_func("rb_str_sublen") | |||
|
|||
unless have_library("re2") | |||
if !static_p and !have_library("re2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we basically using our own variant of have_library
when using MiniPortile2 here because we need to override the linker config to give our vendored dependencies priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we're linking the .a
explicitly instead of using -lre2
. The latter will pick up the system dependency if it's installed in a certain location, and I don't believe there's anything we can do about that.
Does this avoid the problems you ran into with #64 because we control the compilation of libre2 and can ensure it generates position-independent code or is there a scenario where we might run into that again? |
Yes. We set Line 15 in 279d61c
|
Ruby's mkmf appears to list
RbConfig::CONFIG['exec_prefix']/lib
first in the search path when building a C extension. If you have libre2 installed in that path, the linker will use that library instead of the vendored library. This causes an undefined symbol error when using the extension:This commit does two things to ensure the final shared library is compiled statically with the vendored libraries:
For
-L<path>
flags, ensure that anyports
paths are prioritized just in case there are installed libraries that might take precedence.For
-l<lib>
flags, convert the library to the static library with a full path and substitute the absolute static library. For example, -lre2 maps to/path/to/ports/<arch>/libre2/<version>/lib/libre2.a
.