mudge/re2

Switch to MiniPortile2’s mkmf_config feature

mudge opened this issue · 7 comments

mudge commented

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.

stanhu commented

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 pkg-config is broken on Windows, so we need to wait until pkgconf/pkgconf#322 is solved before removing the current static linking mechanism.

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.

mudge commented

FWIW I believe we had a similar problem and @stanhu fixed it in #93

@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.

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.

I'm going to close this now that #138 is merged. The final version ended up being

re2/ext/re2/extconf.rb

Lines 222 to 253 in abdbf04

def static_pkg_config(pc_file, pkg_config_paths)
ENV["PKG_CONFIG_PATH"] = [*pkg_config_paths, ENV["PKG_CONFIG_PATH"]].compact.join(File::PATH_SEPARATOR)
static_library_paths = minimal_pkg_config(pc_file, '--libs-only-L', '--static')
.shellsplit
.map { |flag| flag.delete_prefix('-L') }
$LIBPATH = static_library_paths | $LIBPATH
# Replace all -l flags that can be found in one of the static library
# paths with the absolute path instead.
minimal_pkg_config(pc_file, '--libs-only-l', '--static')
.shellsplit
.each do |flag|
lib = "lib#{flag.delete_prefix('-l')}.#{$LIBEXT}"
if (static_lib_path = static_library_paths.find { |path| File.exist?(File.join(path, lib)) })
$libs << ' ' << File.join(static_lib_path, lib).shellescape
else
$libs << ' ' << flag.shellescape
end
end
append_ldflags(minimal_pkg_config(pc_file, '--libs-only-other', '--static'))
incflags = minimal_pkg_config(pc_file, '--cflags-only-I')
$INCFLAGS = [incflags, $INCFLAGS].join(" ").strip
cflags = minimal_pkg_config(pc_file, '--cflags-only-other')
$CFLAGS = [$CFLAGS, cflags].join(" ").strip
$CXXFLAGS = [$CXXFLAGS, cflags].join(" ").strip
end