-
-
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
Switch to MiniPortile2’s mkmf_config feature #109
Comments
I think this worked: diff --git a/ext/re2/extconf.rb b/ext/re2/extconf.rb
index fd917e6..d38441a 100644
--- a/ext/re2/extconf.rb
+++ b/ext/re2/extconf.rb
@@ -418,17 +418,8 @@ def build_with_vendored_libraries
pkg_config_paths = "#{ENV['PKG_CONFIG_PATH']}#{File::PATH_SEPARATOR}#{pkg_config_paths}" if ENV['PKG_CONFIG_PATH']
ENV['PKG_CONFIG_PATH'] = pkg_config_paths
- pc_file = File.join(re2_recipe.path, 'lib', 'pkgconfig', 're2.pc')
+ re2_recipe.mkmf_config(pkg: 're2', static: 're2')
- raise 'Please install the `pkg-config` utility!' unless find_executable('pkg-config')
-
- # See https://bugs.ruby-lang.org/issues/18490, broken in Ruby 3.1 but fixed in Ruby 3.2.
- flags = xpopen(['pkg-config', '--libs', '--static', pc_file], err: %i[child out], &:read)
-
- raise 'Unable to run pkg-config --libs --static' unless $?.success?
-
- lib_paths = [File.join(abseil_recipe.path, 'lib'), File.join(re2_recipe.path, 'lib')]
- add_static_ldflags(flags, lib_paths)
build_extension(true)
end
diff --git a/ext/re2/recipes.rb b/ext/re2/recipes.rb
index 2ee2698..5c06cdb 100644
--- a/ext/re2/recipes.rb
+++ b/ext/re2/recipes.rb
@@ -1,5 +1,5 @@
PACKAGE_ROOT_DIR = File.expand_path('../..', __dir__)
-REQUIRED_MINI_PORTILE_VERSION = '~> 2.8.4' # keep this version in sync with the one in the gemspec
+REQUIRED_MINI_PORTILE_VERSION = '2.8.5.rc2' # keep this version in sync with the one in the gemspec
def build_recipe(name, version)
require 'rubygems'
diff --git a/re2.gemspec b/re2.gemspec
index dbb64de..57da78f 100644
--- a/re2.gemspec
+++ b/re2.gemspec
@@ -40,5 +40,5 @@ Gem::Specification.new do |s|
s.add_development_dependency("rake-compiler", "~> 1.2.1")
s.add_development_dependency("rake-compiler-dock", "~> 1.3.0")
s.add_development_dependency("rspec", "~> 3.2")
- s.add_runtime_dependency("mini_portile2", "~> 2.8.4") # keep version in sync with extconf.rb
+ s.add_runtime_dependency("mini_portile2", "~> 2.8.5.rc2 ") # keep version in sync with extconf.rb
end However, the main blocker to doing this now is that |
Thanks for kicking the tires. I think there is one more problem with the feature in mini_portile2, which is that there is an unnecessarily high chance of accidentally dynamically linking against system libraries (instead of the vendored static archive). It's fixable, I just need to find the time to do it. |
@flavorjones ACK. Noted in flavorjones/mini_portile#118, will likely adopt a similar approach. |
Per flavorjones/mini_portile#118 (comment), I wonder if this is worth trying again to see if we can delete some code at our end. |
GitHub: #109 Now MiniPortile2 2.8.5 ships with a mkmf_config feature that can handle statically linking libraries for us, switch to using that rather than rolling our own implementation. At the same time, we 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.
I'd be interested in hearing whether it addresses your problem; but I still need to convince myself it will work correctly in 100% of cases and have not had the time to circle back on it yet. |
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.
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.
I'm going to close this now that #138 is merged. The final version ended up being Lines 222 to 253 in abdbf04
|
As per sparklemotion/nokogiri#2977, see if upgrading to mini_portile 2.8.5.rc2 allows us to simplify our
extconf.rb
and the static linking we do there.The text was updated successfully, but these errors were encountered: