search

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

Vendor re2 and abseil #67

Closed stanhu closed 1 year ago

stanhu commented 1 year ago

This pull request:

This makes it possible to ensure all the required dependencies are contained within the C extension rather than depend on system libraries, which may break the extension when they are updated.

The building of these libraries uses mini_portile2 and techniques borrowed from the nokogiri and ruby-magic gems.

glibc 2.29 is required for the arm64 Linux installs.

Closes https://github.com/mudge/re2/issues/61

stanhu commented 1 year ago

@mudge I've tested this on Linux and macOS. If you're happy with this approach, I suggest:

  1. Adding CI to build with bundle exec rake compile -- --disable-system-libraries.
  2. Modifying the Rakefile to include the downloaded .tar.gz files in the gem (such as https://github.com/kwilczynski/ruby-magic/blob/b781dd6b03f45e2e7c31939f9c034d568992a27e/Rakefile#L126-L137).
  3. Consider building precompiled native gems with rack-compiler.

UPDATE: First two are complete.

mudge commented 1 year ago

This is a huge amount of work; thanks for putting in the effort, @stanhu!

Re your checklist:

  1. Agreed, it looks like you have the matrix set up but we should be testing all supported Ruby versions against supported system re2 ABI versions as well as all Ruby versions against the vendored dependencies.
  2. This was going to be my first question: could we vendor the dependencies directly so users only need to download a single gem?
  3. This would be the ultimate goal: users don't need to compile anything at all and just download a binary that Just Works on their system of choice.

With the dependency on MiniPortile meaning we drop support for Ruby < 2.3, I'd consider this worthy of a major version bump (i.e. re2 2.0) and, if we're breaking compatibility, perhaps it would make sense for the vendored libraries to be the default. That better justifies including the sources otherwise it seems a little bloated to include dependencies that will never be used. It also makes the precompiled gems make a little more sense: the default expectation is that you can install the gem without having to install re2 or abseil yourself.

stanhu commented 1 year ago

@mudge Thanks for the feedback!

Agreed, it looks like you have the matrix set up but we should be testing all supported Ruby versions against supported system re2 ABI versions as well as all Ruby versions against the vendored dependencies.

I think that's what happening now in https://github.com/mudge/re2/pull/67/checks. Was there something I missed?

I would hesitate making the vendored libraries the default until precompiled gems are available for a number of reasons:

  1. Building abseil takes a minute or two.
  2. Users may not have a recent enough C++ compiler and/or cmake.

I should note that on ARM64 platforms, precompiled gems built with rake-compiler-dock require glibc 2.29 (see https://github.com/rake-compiler/rake-compiler-dock/pull/68).

I have a change I'm testing right now that would generate precompiled gems, but then you'll probably want to have some automation to build and push these gems automatically (such as https://github.com/y-crdt/yrb/blob/main/.github/workflows/gem.yml).

How would you like to proceed? Do you want to tackle everything in this pull request, or get this merged first?

mudge commented 1 year ago

I think that's what happening now in https://github.com/mudge/re2/pull/67/checks. Was there something I missed?

No no, just that we agree on what we should be testing.

(I suppose it is wishful thinking to only require Ruby >= 2.3 if you're opting into the vendored dependencies as we need the gem dependency at install time.)

I would hesitate making the vendored libraries the default until precompiled gems are available for a number of reasons:

  1. Building abseil takes a minute or two.
  2. Users may not have a recent enough C++ compiler and/or cmake.

Agreed, I was imagining that we'd only make it the default when and only when precompiled gems are available. That way, the common user experience is a fast install but it degrades to a slow compile if a precompiled gem isn't available for your platform.

I should note that on ARM64 platforms, precompiled gems built with rake-compiler-dock require glibc 2.29 (see rake-compiler/rake-compiler-dock#68).

I'm unfamiliar with precompiled gems, I don't suppose there's a way to only install one if your platform meets more than an CPU architecture requirement?

I have a change I'm testing right now that would generate precompiled gems, but then you'll probably want to have some automation to build and push these gems automatically (such as https://github.com/y-crdt/yrb/blob/main/.github/workflows/gem.yml).

Agreed.

How would you like to proceed? Do you want to tackle everything in this pull request, or get this merged first?

Would it be helpful for me to make a new v2.0.x branch and we can start merging PRs into that?

stanhu commented 1 year ago

Would it be helpful for me to make a new v2.0.x branch and we can start merging PRs into that?

Either way.

I'm still ironing out some cross-compiling issues for aarch64.

stanhu commented 1 year ago

@mudge Ok, I fixed the cross-compilation issues. Turns out using cmake is a bit trickier than the standard configure and make. If you run bundle exec rake gem:native now, this should build precompiled gems for these platforms:

~I'm not sure what's going on with the Windows builds yet, but it should be possible given grpc compiles abseil and ships with precompiled Windows gems.~ Fixed now.

Also, the pre-compiled gems only ship with Ruby 2.7 - 3.2 binaries. Do you want to include 2.3 and up?

flavorjones commented 1 year ago

:wave: @mudge asked me to take a look. I can't see anything that stands out as needing fixing, though I made a few comments.

What I think is missing, though, is more test coverage.

I would recommend round-trip tests for the precompiled gems:

And a similar round-trip test for the vanilla "ruby" platform gem, just to ensure something's not been broken, e.g. the gemspec file manifest.

Finally -- and this is optional and extra credit -- it may be worth writing tests to verify

I'd be happy to help write these! I have freshly-loaded context from gemifying the YARP parser. Just let me know what you want assistance with.

stanhu commented 1 year ago

@flavorjones Thanks so much for the review and all your work on mini_portile2!

What I think is missing, though, is more test coverage.

Very true! I just worked out all the kinks with cross-compiling with cmake yesterday.

I would recommend round-trip tests for the precompiled gems

Thanks for the links! That will help move this faster.

also include musl target systems for the linux builds to make sure you won't get bitten by any glibc/musl incompatibilities.

I was wondering why Nokogiri didn't ship a musl build. Is that because the existing builds are compatible? I attempted to ship a musl gem in another project, but we found we had to require RubyGems >= v3.3.222 because some of our platforms attempted to install the musl version instead of glibc due to https://github.com/rubygems/rubygems/pull/5852.

And a similar round-trip test for the vanilla "ruby" platform gem, just to ensure something's not been broken, e.g. the gemspec file manifest.

Would love to help with this. I've spent enough time working on this that I'd appreciate more help.

stanhu commented 1 year ago

I was wondering why Nokogiri didn't ship a musl build. Is that because the existing builds are compatible?

To answer my own question:, it looks like on Alpine 3.17:

/ # ldd ./usr/lib/ruby/gems/3.1.0/gems/nokogiri-1.15.3-x86_64-linux/lib/nokogiri/3.1/nokogiri.so | more
        /lib/ld-musl-x86_64.so.1 (0x7f42fbe51000)
        libm.so.6 => /lib/ld-musl-x86_64.so.1 (0x7f42fbe51000)
        libdl.so.2 => /lib/ld-musl-x86_64.so.1 (0x7f42fbe51000)
Error relocating ./usr/lib/ruby/gems/3.1.0/gems/nokogiri-1.15.3-x86_64-linux/lib/nokogiri/3.1/nokogiri.so        libc.so.6 => /lib/ld-musl-x86_64.so.1 (0x7f42fbe51000)
<snip>

However, since abseil needs libstdc++ and other libraries, the re2 precompiled gem currently fails miserably:

/ # ldd /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so
        /lib/ld-musl-x86_64.so.1 (0x7f4720899000)
        librt.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f4720899000)
        libruby.so.3.1 => /usr/lib/libruby.so.3.1 (0x7f472026a000)
Error loading shared library libstdc++.so.6: No such file or directory (needed by /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so)
Error loading shared library ld-linux-x86-64.so.2: No such file or directory (needed by /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so)
Error loading shared library libgcc_s.so.1: No such file or directory (needed by /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so)

It looks like running apk add stdc++ helps, but I get:

/ # ldd /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so
        /lib/ld-musl-x86_64.so.1 (0x7f2a67c04000)
        librt.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f2a67c04000)
        libruby.so.3.1 => /usr/lib/libruby.so.3.1 (0x7f2a675d5000)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x7f2a67387000)
        libm.so.6 => /lib/ld-musl-x86_64.so.1 (0x7f2a67c04000)
        libc.so.6 => /lib/ld-musl-x86_64.so.1 (0x7f2a67c04000)
Error loading shared library ld-linux-x86-64.so.2: No such file or directory (needed by /usr/lib/ruby/gems/3.1.0/gems/re2-1.7.0-x86_64-linux/lib/re2.so)
        libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x7f2a67369000)
        libpthread.so.0 => /lib/ld-musl-x86_64.so.1 (0x7f2a67c04000)
        libz.so.1 => /lib/libz.so.1 (0x7f2a6734f000)
        libgmp.so.10 => /usr/lib/libgmp.so.10 (0x7f2a672e8000)

Running apk add gcompat seems to solve that at least.

Still, this isn't a great experience for Alpine users. Do we need to ship a precompiled musl extension, or is there a better way?

stanhu commented 1 year ago

Still, this isn't a great experience for Alpine users. Do we need to ship a precompiled musl extension, or is there a better way?

I checked what the grpc gem does. The precompiled extension doesn't appear to need libstdc++, but it does need apk add gcompat to work on Alpine.

flavorjones commented 1 year ago

I was wondering why Nokogiri didn't ship a musl build. Is that because the existing builds are compatible?

Nokogiri builds are compatible with musl (and we did jump through some hoops to do that), but do require gcompat on some systems. You found the relatively recent feature added that supports non-glibc linux platforms, but I haven't had to play with it yet. Formal musl support has been a low priority compared to some of the other work that needs to be done. :shrug:

Do we really want to support all the way to Ruby 2.3.0?

I recommend only precompiling for supported Rubies. Nokogiri and Sqlite3 have adopted a policy of only shipping 4 versions max (3 in current support, plus the one most recently out-of-support).

A precompiled extension is a good "carrot" to encourage people to upgrade Ruby. (Also, my experience has been that a disproportionate number of support issues come from people running old versions of Ruby.)

flavorjones commented 1 year ago

Would love to help with this. I've spent enough time working on this that I'd appreciate more help.

Please point me at what you'd like me to do. I'll have some time over the next few days, and I will try to help as much as I can. Just don't want to duplicate anything you've already done!

stanhu commented 1 year ago

Please point me at what you'd like me to do. I'll have some time over the next few days, and I will try to help as much as I can. Just don't want to duplicate anything you've already done!

@flavorjones I'm going to take a break from this work since I have to focus on other things, but I would love help with all the round-trip tests you mentioned!

flavorjones commented 1 year ago

@stanhu would love to help ... you want me to just continue this branch in another PR? or add me as a collaborator on your fork?

stanhu commented 1 year ago

@flavorjones I'll just add you to my fork. At some point perhaps @mudge can just merge this to the v2.0.x branch and work from there.

mudge commented 1 year ago

I'll merge this into v2.0.x now to make it a bit easier to work in parallel. Please feel free to raise issues for the 2.0 milestone if it'd be easier to keep track of everything that way.

mudge commented 1 year ago

I've raised the issues mentioned above and attached them to the 2.0 milestone: https://github.com/mudge/re2/milestone/1

flavorjones commented 1 year ago

:+1: Just a note that it may be a few days before I'm able to circle back on the testing issues (I have some other OSS work that is blocking other folks that I need to prioritize), but I will get there.

mudge commented 1 year ago

@stanhu I just tried running bundle exec rake clobber compile on the v2.0.x branch on my 14" MacBook Pro (M1 Max) but got the following error:

$ bundle exec rake clobber compile
rm -r pkg
mkdir -p tmp/arm64-darwin22/re2/3.2.2
cd tmp/arm64-darwin22/re2/3.2.2
/Users/mudge/.rubies/ruby-3.2.2/bin/ruby -I. -r.rake-compiler-siteconf.rb ../../../../ext/re2/extconf.rb --disable-system-libraries
Building re2 using packaged libraries.
Using mini_portile version 2.8.2
checking for arm64-apple-darwin22-clang... no
checking for clang... yes
checking for arm64-apple-darwin22-clang++... no
checking for clang++... yes
---------- IMPORTANT NOTICE ----------
Building re2 with a packaged version of abseil-20230125.3.
Configuration options: -DCMAKE_CXX_STANDARD\=17 -DCMAKE_POSITION_INDEPENDENT_CODE\=ON -DCMAKE_INSTALL_LIBDIR\=lib -DCMAKE_SYSTEM_PROCESSOR\=arm64 -DCMAKE_SYSTEM_NAME\=Darwin -DCMAKE_C_COMPILER\=clang -DCMAKE_CXX_COMPILER\=clang++ -DABSL_PROPAGATE_CXX_STD\=ON
Extracting 20230125.3.tar.gz into tmp/arm64-apple-darwin22/ports/abseil/20230125.3... OK
Running 'configure' for abseil 20230125.3... OK
Running 'compile' for abseil 20230125.3... OK
Running 'install' for abseil 20230125.3... OK
Activating abseil 20230125.3 (from /Users/mudge/Projects/re2/ports/arm64-apple-darwin22/abseil/20230125.3)...
Using mini_portile version 2.8.2
checking for arm64-apple-darwin22-clang... no
checking for clang... yes
checking for arm64-apple-darwin22-clang++... no
checking for clang++... yes
---------- IMPORTANT NOTICE ----------
Building re2 with a packaged version of libre2-2023-07-01.
Configuration options: -DCMAKE_CXX_STANDARD\=17 -DCMAKE_POSITION_INDEPENDENT_CODE\=ON -DCMAKE_INSTALL_LIBDIR\=lib -DCMAKE_SYSTEM_PROCESSOR\=arm64 -DCMAKE_SYSTEM_NAME\=Darwin -DCMAKE_C_COMPILER\=clang -DCMAKE_CXX_COMPILER\=clang++ -DCMAKE_PREFIX_PATH\=/Users/mudge/Projects/re2/ports/arm64-apple-darwin22/abseil/20230125.3 -DCMAKE_CXX_FLAGS\=-DNDEBUG
Extracting re2-2023-07-01.tar.gz into tmp/arm64-apple-darwin22/ports/libre2/2023-07-01... OK
Running 'configure' for libre2 2023-07-01... OK
Running 'compile' for libre2 2023-07-01... OK
Running 'install' for libre2 2023-07-01... OK
Activating libre2 2023-07-01 (from /Users/mudge/Projects/re2/ports/arm64-apple-darwin22/libre2/2023-07-01)...
checking for whether -L/Users/mudge/Projects/re2/ports/arm64-apple-darwin22/libre2/2023-07-01/lib is accepted as LDFLAGS... yes
checking for whether -L/Users/mudge/Projects/re2/ports/arm64-apple-darwin22/abseil/20230125.3/lib is accepted as LDFLAGS... yes
checking for whether -pthread is accepted as LDFLAGS... yes
checking for whether -lre2 is accepted as LDFLAGS... yes
checking for whether -labsl_flags is accepted as LDFLAGS... yes
checking for whether -labsl_flags_internal is accepted as LDFLAGS... yes
checking for whether -labsl_flags_marshalling is accepted as LDFLAGS... yes
checking for whether -labsl_flags_reflection is accepted as LDFLAGS... yes
checking for whether -labsl_flags_private_handle_accessor is accepted as LDFLAGS... yes
checking for whether -labsl_flags_commandlineflag is accepted as LDFLAGS... yes
checking for whether -labsl_flags_commandlineflag_internal is accepted as LDFLAGS... yes
checking for whether -labsl_flags_config is accepted as LDFLAGS... yes
checking for whether -labsl_flags_program_name is accepted as LDFLAGS... yes
checking for whether -labsl_cord is accepted as LDFLAGS... yes
checking for whether -labsl_cordz_info is accepted as LDFLAGS... yes
checking for whether -labsl_cord_internal is accepted as LDFLAGS... yes
checking for whether -labsl_cordz_functions is accepted as LDFLAGS... yes
checking for whether -labsl_cordz_handle is accepted as LDFLAGS... yes
checking for whether -labsl_crc_cord_state is accepted as LDFLAGS... yes
checking for whether -labsl_crc32c is accepted as LDFLAGS... yes
checking for whether -labsl_crc_internal is accepted as LDFLAGS... yes
checking for whether -labsl_crc_cpu_detect is accepted as LDFLAGS... yes
checking for whether -labsl_hash is accepted as LDFLAGS... yes
checking for whether -labsl_city is accepted as LDFLAGS... yes
checking for whether -labsl_bad_variant_access is accepted as LDFLAGS... yes
checking for whether -labsl_low_level_hash is accepted as LDFLAGS... yes
checking for whether -labsl_raw_hash_set is accepted as LDFLAGS... yes
checking for whether -labsl_hashtablez_sampler is accepted as LDFLAGS... yes
checking for whether -labsl_exponential_biased is accepted as LDFLAGS... yes
checking for whether -labsl_bad_optional_access is accepted as LDFLAGS... yes
checking for whether -labsl_str_format_internal is accepted as LDFLAGS... yes
checking for whether -labsl_synchronization is accepted as LDFLAGS... yes
checking for whether -labsl_graphcycles_internal is accepted as LDFLAGS... yes
checking for whether -labsl_stacktrace is accepted as LDFLAGS... yes
checking for whether -labsl_symbolize is accepted as LDFLAGS... yes
checking for whether -labsl_debugging_internal is accepted as LDFLAGS... yes
checking for whether -labsl_demangle_internal is accepted as LDFLAGS... yes
checking for whether -labsl_malloc_internal is accepted as LDFLAGS... yes
checking for whether -labsl_time is accepted as LDFLAGS... yes
checking for whether -labsl_civil_time is accepted as LDFLAGS... yes
checking for whether -labsl_strings is accepted as LDFLAGS... yes
checking for whether -labsl_strings_internal is accepted as LDFLAGS... yes
checking for whether -labsl_base is accepted as LDFLAGS... yes
checking for whether -labsl_spinlock_wait is accepted as LDFLAGS... yes
checking for whether -labsl_int128 is accepted as LDFLAGS... yes
checking for whether -labsl_throw_delegate is accepted as LDFLAGS... yes
checking for whether -labsl_raw_logging_internal is accepted as LDFLAGS... yes
checking for whether -labsl_log_severity is accepted as LDFLAGS... yes
checking for whether -labsl_time_zone is accepted as LDFLAGS... yes
checking for -lstdc++... yes
checking for stdint.h... yes
checking for rb_str_sublen()... yes
checking for -lre2... yes
checking for re2 that requires explicit C++ version flag... yes
checking for re2 that compiles with c++20 standard... yes
checking for RE2::Match() with endpos argument... yes
checking for RE2::Set::Match() with error information... yes
creating Makefile
cd -
cd tmp/arm64-darwin22/re2/3.2.2
/usr/bin/make
compiling ../../../../ext/re2/re2.cc
../../../../ext/re2/re2.cc:10:10: fatal error: 're2/re2.h' file not found
#include <re2/re2.h>
         ^~~~~~~~~~~
1 error generated.
make: *** [re2.o] Error 1
rake aborted!
Command failed with status (2): [/usr/bin/make...]
/Users/mudge/.gem/ruby/3.2.2/gems/rake-compiler-1.2.3/lib/rake/extensiontask.rb:201:in `block (2 levels) in define_compile_tasks'
/Users/mudge/.gem/ruby/3.2.2/gems/rake-compiler-1.2.3/lib/rake/extensiontask.rb:200:in `block in define_compile_tasks'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli/exec.rb:58:in `load'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli/exec.rb:58:in `kernel_load'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli/exec.rb:23:in `run'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli.rb:492:in `exec'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli.rb:34:in `dispatch'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/cli.rb:28:in `start'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/exe/bundle:45:in `block in <top (required)>'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
/Users/mudge/.gem/ruby/3.2.2/gems/bundler-2.4.12/exe/bundle:33:in `<top (required)>'
/Users/mudge/.gem/ruby/3.2.2/bin/bundle:25:in `load'
/Users/mudge/.gem/ruby/3.2.2/bin/bundle:25:in `<main>'
Tasks: TOP => compile => compile:arm64-darwin22 => compile:re2:arm64-darwin22 => copy:re2:arm64-darwin22:3.2.2 => tmp/arm64-darwin22/re2/3.2.2/re2.bundle
(See full trace by running task with --trace)

Am I missing something?

stanhu commented 1 year ago

@mudge You probably need the changes in #76, namely aa0bdb616498e02e705e5af6df20a03c520c7872.

stanhu commented 1 year ago

I extracted the essential changes in https://github.com/mudge/re2/pull/76 into https://github.com/mudge/re2/pull/77.