Skip to content
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

Closed
mudge opened this issue Sep 24, 2023 · 7 comments
Closed

Switch to MiniPortile2’s mkmf_config feature #109

mudge opened this issue Sep 24, 2023 · 7 comments
Assignees
Labels
packaging Issues related to the building of the gem (including precompilation)

Comments

@mudge
Copy link
Owner

mudge commented Sep 24, 2023

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.

@mudge mudge added the packaging Issues related to the building of the gem (including precompilation) label Sep 25, 2023
@stanhu
Copy link
Collaborator

stanhu commented Oct 17, 2023

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.

@flavorjones
Copy link

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
Copy link
Owner Author

mudge commented Oct 22, 2023

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

@flavorjones
Copy link

@flavorjones ACK. Noted in flavorjones/mini_portile#118, will likely adopt a similar approach.

@mudge mudge changed the title Test against mini_portile 2.8.5.rc2 Switch to MiniPortile2’s mkmf_config feature Nov 5, 2023
@mudge
Copy link
Owner Author

mudge commented Mar 10, 2024

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.

mudge added a commit that referenced this issue Mar 10, 2024
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.
@flavorjones
Copy link

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 added a commit that referenced this issue Apr 1, 2024
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.
mudge added a commit that referenced this issue Apr 1, 2024
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.
@mudge
Copy link
Owner Author

mudge commented Apr 1, 2024

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

@mudge mudge closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Issues related to the building of the gem (including precompilation)
Projects
None yet
Development

No branches or pull requests

3 participants