Learn from MiniPortile2's mkmf_config to simplify building vendored dependencies #138
Conversation
mudge
force-pushed
the
miniportile-mkmf-config
branch
from
6ddb2cb
to
ac14e8f
Compare
mudge
changed the title
|
@stanhu would you mind taking a look at this to double check it makes sense to you (the diff is a bit noisy so it might better to just read https://github.com/mudge/re2/blob/e72914f7b415adf71205c25a83c75c0769ecbd1f/ext/re2/extconf.rb top to bottom)? This has mostly been a learning experience for me to fully understand how the static linking previously worked and how we can combine it with @flavorjones' efforts upstream in MiniPortile2 (as well as Ruby's own MakeMakefile's We now rely entirely on the various outputs of
This completely replaces our usage of @flavorjones in case it is of any interest, most of the above logic is consolidated in a new |
|
I’ve done some cursory testing of the Darwin arm64, Linux aarch64 and x86_64 gems with no system RE2 or Abseil present and confirm they seem to work fine. Update: |
|
Moving this back into draft until I can be certain it doesn’t introduce any regressions. As you can see from the churn here, we need something to ground the changes so I propose comparing the state of CXXFLAGS, CPPFLAGS, INCFLAGS, LDFLAGS, LIBPATH, ARCH_FLAG, LOCAL_LIBS and LIBS. |
|
|
||
| if !static_p and !have_library("re2") |
There was a problem hiding this comment.
Not running have_library("re2") is important during the static build otherwise MakeMakefile will add lre2 to LIBS to the Makefile increasing the risk we'll link to the wrong version.
mudge
force-pushed
the
miniportile-mkmf-config
branch
2 times, most recently
from
4cd3700
to
ba7db0c
Compare
ext/re2/extconf.rb
Outdated
| lib_paths.each do |path| | ||
| static_lib = File.join(path, filename) | ||
| def static_pkg_config(pc_file, pkg_config_paths) | ||
| # on macOS, pkg-config will not return --cflags without this |
There was a problem hiding this comment.
Hmm, I don't seem to need that environment variable:
% PKG_CONFIG_PATH=./ports/arm64-apple-darwin22/abseil/20230125.3/lib/pkgconfig:./ports/aarch64-apple-darwin22.6.0/libre2/2023-07-01/lib/pkgconfig pkg-config --libs-only-L --static re2
-L/Users/stanhu/github/re2/ports/arm64-apple-darwin22/abseil/20230125.3/lib -L/Users/stanhu/github/re2/ports/aarch64-apple-darwin22.6.0/libre2/2023-07-01/lib
stanhu@jet-arm re2 % PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=t PKG_CONFIG_PATH=./ports/arm64-apple-darwin22/abseil/20230125.3/lib/pkgconfig:./ports/aarch64-apple-darwin22.6.0/libre2/2023-07-01/lib/pkgconfig pkg-config --libs-only-L --static re2
-L/Users/stanhu/github/re2/ports/arm64-apple-darwin22/abseil/20230125.3/lib -L/Users/stanhu/github/re2/ports/aarch64-apple-darwin22.6.0/libre2/2023-07-01/libThere was a problem hiding this comment.
https://linux.die.net/man/1/pkg-config says:
PKG_CONFIG_ALLOW_SYSTEM_CFLAGS
Don't strip -I/usr/include out of cflags.
PKG_CONFIG_ALLOW_SYSTEM_LIBS
Don't strip -L/usr/lib out of libs
Is /usr/include being included?
There was a problem hiding this comment.
I think you're right: I took this from MiniPortile2 but given we don't want to link to /usr/include, we can probably skip it unless @flavorjones knows better?
|
In any case, this looks great to me. Thanks for this! |
|
Thanks, @stanhu. I think the only thing missing by switching to using |
GitHub: #109 MiniPortile2 2.8.5 ships with a `mkmf_config` feature that can handle statically linking libraries for us, however it doesn't currently work with our use case due to Abseil shipping with over 180 separate `.pc` files. That said, we can still update how we statically link RE2 into the gem based on MiniPortile2's strategy and how Ruby's own MakeMakefile's `pkg_config` works. The key is that we now rely on the output of the following `pkg-config` commands to populate `$LIBPATH`, `$libs`, `$LDFLAGS`, `$INCFLAGS`, `$CFLAGS` and `$CXXFLAGS` respectively: * `pkg-config --libs-only-L --static` * `pkg-config --libs-only-l --static` * `pkg-config --libs-only-other --static` * `pkg-config --cflags-only-I --static` * `pkg-config --cflags-only-other --static` We transform any libraries into static ones by replacing them with their absolute path wherever possible. Note we _must not_ use MakeMakefile's `dir_config` to avoid accidentally adding a non-absolute (and therefore dynamic) reference to RE2 which risks accidentally linking against the wrong version of the library, especially if it is found in Ruby's default `exec_prefix` (e.g. `/usr/local`). We also take a leaf from the Ruby SQLite3 gem's extconf (https://github.com/sparklemotion/sqlite3-ruby/blob/ae5c13996f936fce07e8a5e9fb6aaf2ede5d82b7/ext/sqlite3/extconf.rb#L113) and re-organise the configuration logic in our extconf.rb into a class rather than a series of global functions. Thanks to @flavorjones for his work on MiniPortile2 and @stanhu for reviewing a draft of this change.
To catch any accidental errors mutating frozen strings, enable them by default in CI. See https://ruby.social/@byroot/112177840865649404
mudge
force-pushed
the
miniportile-mkmf-config
branch
from
fd7f6ff
to
792c992
Compare
GitHub: #109
MiniPortile2 2.8.5 ships with a mkmf_config feature that can handle statically linking libraries for us, however it doesn't currently work with our use case due to Abseil shipping with over 180 separate
.pcfiles. That said, we can still update how we statically link RE2 into the gem based on MiniPortile2's strategy.We also take a leaf from the Ruby SQLite3 gem's extconf (https://github.com/sparklemotion/sqlite3-ruby/blob/ae5c13996f936fce07e8a5e9fb6aaf2ede5d82b7/ext/sqlite3/extconf.rb#L113) and re-organise the configuration logic in our extconf.rb into a class rather than a series of global functions.