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 static linking when system libraries installed #93

Merged
merged 2 commits into from
Sep 9, 2023

Conversation

stanhu
Copy link
Collaborator

@stanhu stanhu commented Sep 8, 2023

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.

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.
@mudge
Copy link
Owner

mudge commented Sep 9, 2023

Is any part of this worth upstreaming into MiniPortile2 or is it specific to our setup?

@stanhu
Copy link
Collaborator Author

stanhu commented Sep 9, 2023

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 mkmf could be improved to make it easier to statically link external libraries, but I suspect it's not that common to do this.

@@ -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
Copy link
Owner

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?

Copy link
Collaborator Author

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")
Copy link
Owner

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?

Copy link
Collaborator Author

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.

@mudge
Copy link
Owner

mudge commented Sep 9, 2023

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?

@stanhu
Copy link
Collaborator Author

stanhu commented Sep 9, 2023

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 cmake with -DCMAKE_POSITION_INDEPENDENT_CODE=ON' in

'-DCMAKE_POSITION_INDEPENDENT_CODE=ON',

@mudge mudge merged commit 3d093ad into mudge:v2.0.x Sep 9, 2023
126 checks passed
@stanhu stanhu mentioned this pull request Sep 20, 2023
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.

2 participants