mudge / re2

Ruby bindings to RE2, a "fast, safe, thread-friendly alternative to backtracking regular expression engines like those used in PCRE, Perl, and Python".
http://mudge.name/re2/
BSD 3-Clause "New" or "Revised" License
130 stars 13 forks source link

Switch to MiniPortile2’s mkmf_config feature #109

Closed mudge closed 7 months ago

mudge commented 1 year ago

As per https://github.com/sparklemotion/nokogiri/pull/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 1 year ago

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 https://github.com/pkgconf/pkgconf/issues/322 is solved before removing the current static linking mechanism.

flavorjones commented 1 year ago

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 1 year ago

FWIW I believe we had a similar problem and @stanhu fixed it in https://github.com/mudge/re2/pull/93

flavorjones commented 1 year ago

@flavorjones ACK. Noted in https://github.com/flavorjones/mini_portile/issues/118, will likely adopt a similar approach.

mudge commented 8 months ago

Per https://github.com/flavorjones/mini_portile/issues/118#issuecomment-1958583207, I wonder if this is worth trying again to see if we can delete some code at our end.

flavorjones commented 7 months ago

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.

mudge commented 7 months ago

I'm going to close this now that https://github.com/mudge/re2/pull/138 is merged. The final version ended up being https://github.com/mudge/re2/blob/abdbf04229d4abbe259f37615a8b7fad0fe228e1/ext/re2/extconf.rb#L222-L253