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
129 stars 13 forks source link

Learn from MiniPortile2's mkmf_config to simplify building vendored dependencies #138

Closed mudge closed 5 months ago

mudge commented 6 months ago

GitHub: https://github.com/mudge/re2/issues/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.

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.

mudge commented 5 months ago

@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 dir_config and pkg_config) to set the bare minimum amount of configuration to statically link RE2 into the gem.

We now rely entirely on the various outputs of pkg-config to set:

This completely replaces our usage of dir_config when using the vendored libraries.

@flavorjones in case it is of any interest, most of the above logic is consolidated in a new static_pkg_config method.

mudge commented 5 months ago

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: ~@stanhu this fails during the final linking stage if I put libre2.a in Ruby's exec_prefix so it's clearly missing something essential to prevent that.~ Switching from $libs to $LDFLAGS seems to fix things.

mudge commented 5 months ago

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.

stanhu commented 5 months ago

In any case, this looks great to me. Thanks for this!

mudge commented 5 months ago

Thanks, @stanhu. I think the only thing missing by switching to using pkg-config's various --libs-only- and --cflags-only- flags is that we should also be sure to include --libs-only-other (which includes -pthread for re2.pc). Ruby's own pkg_config uses --libs and then removes anything it also finds in --libs-only-l but I think using the more specific flags should simplify things for us (as it reduces the chance of accidentally including a library twice).