Skip to content

Fix static linking when system libraries installed - #93

Merged
mudge merged 2 commits into
mudge:v2.0.xfrom
stanhu:sh-static-link-fix
Sep 9, 2023
Merged

mudge merged 2 commits into
mudge:v2.0.xfrom
stanhu:sh-static-link-fix

Conversation

@stanhu

@stanhu stanhu commented Sep 8, 2023

Copy link
Copy Markdown
Collaborator

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

mudge commented Sep 9, 2023

Copy link
Copy Markdown
Owner

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

@stanhu

stanhu commented Sep 9, 2023

Copy link
Copy Markdown
Collaborator Author

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.

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
Copy Markdown
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
Copy Markdown
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.

Comment thread ext/re2/extconf.rb
have_func("rb_str_sublen")

unless have_library("re2")
if !static_p and !have_library("re2")

Copy link
Copy Markdown
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
Copy Markdown
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

mudge commented Sep 9, 2023 •

Copy link
Copy Markdown
Owner

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

stanhu commented Sep 9, 2023 •

Copy link
Copy Markdown
Collaborator Author

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',

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