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

Upgrade abseil to 20230802.1 #95

Closed mudge closed 1 year ago

mudge commented 1 year ago

RE2 2023-09-01 was released a week ago and there is a more recent LTS version of Abseil for us to use. However, when I initially pushed this to our v2.0.x branch, it looks like there are quite a few build failures to investigate.

mudge commented 1 year ago

If it’s a distraction right now, we could skip the abseil upgrade but it’d be good to know why it doesn’t work when the January release does.

stanhu commented 1 year ago

It looks like there are two main issues:

  1. macOS (https://github.com/mudge/re2/actions/runs/6130653443/job/16639942206): absel::optional is aliased to C++17 std::optional, but the call to value() (introduced in abseil-cpp via https://github.com/abseil/abseil-cpp/commit/751ade00ee347abef5dac7248db851e3f2012e14) is only available starting in macOS 10.14. Even though the macOS SDK 11.1 is used, I'd have to presume the compiler tries to target a lower minimum target version:
 [ 97%] Building CXX object absl/status/CMakeFiles/status.dir/status.cc.o
/tmp/d20230909-93-us36r9/tmp/arm64-apple-darwin/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/status/status.cc:126:51: error: 'value' is unavailable: introduced in macOS 10.14
  if (index.has_value()) return (*payloads)[index.value()].payload;
                                                  ^

I think we might be able to set ABSL_OPTION_USE_STD_OPTIONAL to 0 to disable this aliasing, or consider raising the macOS minimum version. clang --help shows this option:

-mmacosx-version-min=<value>
                      Set Mac OS X deployment target
  1. Windows (https://github.com/mudge/re2/actions/runs/6130653443/job/16639942231):
[ 37%] Building CXX object absl/synchronization/CMakeFiles/synchronization.dir/internal/create_thread_identity.cc.obj
In file included from /tmp/d20230909-93-l6kcab/tmp/x86_64-w64-mingw32/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/synchronization/internal/create_thread_identity.cc:21:
/tmp/d20230909-93-l6kcab/tmp/x86_64-w64-mingw32/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/synchronization/internal/waiter.h:44:2: error: #error ABSL_WAITER_MODE is undefined
   44 | #error ABSL_WAITER_MODE is undefined
      |  ^~~~~
/tmp/d20230909-93-l6kcab/tmp/x86_64-w64-mingw32/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/synchronization/internal/waiter.h:51:5: warning: "ABSL_WAITER_MODE" is not defined, evaluates to 0 [-Wundef]
   51 | #if ABSL_WAITER_MODE == ABSL_WAITER_MODE_FUTEX
      |     ^~~~~~~~~~~~~~~~
In file included from /tmp/d20230909-93-l6kcab/tmp/x86_64-w64-mingw32/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/synchronization/internal/create_thread_identity.cc:21:
/tmp/d20230909-93-l6kcab/tmp/x86_64-w64-mingw32/ports/abseil/20230802.0/abseil-cpp-20230802.0/absl/synchronization/internal/waiter.h:52:16: error: 'FutexWaiter' does not name a type
   52 | using Waiter = FutexWaiter;
      |                ^~~~~~~~~~~
make[2]: *** [absl/synchronization/CMakeFiles/synchronization.dir/build.make:92: absl/synchronization/CMakeFiles/synchronization.dir/internal/create_thread_identity.cc.obj] Error 1
make[1]: *** [CMakeFiles/Makefile2:4540: absl/synchronization/CMakeFiles/synchronization.dir/all] Error 2

There appears to be a workaround: https://github.com/abseil/abseil-cpp/issues/1510#issuecomment-1694249704.

stanhu commented 1 year ago

@mudge I think this is ready to go.

mudge commented 1 year ago

Updated for the recently released 20230802.1 (~note I removed https://github.com/mudge/re2/pull/95/commits/c284a7e2bc9c873ce756b4b39e17568d40f8b6ea just to see if that regression has been fixed but we may need to restore it~ restored as it looks like it is still a problem).

mudge commented 1 year ago

@stanhu any chance you could take a look at this? The Windows failures seem different to with patch level 0.

undefined reference to `absl::lts_20230802::synchronization_internal::Win32Waiter::Win32Waiter()
mudge commented 1 year ago

Out of interest, is MacPorts providing a version of Abseil that works with versions of macOS older than 10.14? See https://github.com/judaew/macports-ports/blob/master/devel/abseil/Portfile

stanhu commented 1 year ago

std::optional was added in C++17, and I see that they have set C++14 in https://github.com/judaew/macports-ports/blob/c536e357838df685f32fdedc7b74dd5d4f0325ce/devel/abseil/Portfile#L68. This might avoid the need to upgrade macOS. Maybe for macOS we can use C++14.

mudge commented 1 year ago

std::optional was added in C++17, and I see that they have set C++14 in https://github.com/judaew/macports-ports/blob/c536e357838df685f32fdedc7b74dd5d4f0325ce/devel/abseil/Portfile#L68. This might avoid the need to upgrade macOS. Maybe for macOS we can use C++14.

Good spot. Given Abseil only needs C++14 (and RE2 follows suit), I’ve set our MiniPortile recipes to only target that standard and it seems to fix the macOS build without dropping support for older versions.

stanhu commented 1 year ago

Did this branch drop 0b57520e4f8b689bb6212f7d20f73d154a26c057?

mudge commented 1 year ago

Yes, I've restored it now (I was trying to see what was and wasn't required with patch level 1).

mudge commented 1 year ago

The build is now green and (aside from the necessary workaround for https://github.com/abseil/abseil-cpp/issues/1510) I'm happier with the diff especially as we're not dropping support for any existing users.

mudge commented 1 year ago

If you're happy, I'd be tempted to merge this but not include it in a release until RE2 2023-10-01 is out (assuming it has a monthly release cadence now).

mudge commented 1 year ago

I've raised https://github.com/mudge/re2/pull/111 to partially automate this sort of thing in future: it's set to run on the first of the month and raise a PR with an updated dependencies.yml (if there are any releases). I'm tempted to merge that, trigger it manually and then cherry pick these commits onto the PR that is automatically raised.

stanhu commented 1 year ago

If you're happy, I'd be tempted to merge this but not include it in a release until RE2 2023-10-01 is out (assuming it has a monthly release cadence now).

Sounds good!

https://github.com/mudge/re2/pull/112 didn't run CI. Does the user need to be approved?

mudge commented 1 year ago

I think it’s due to https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#triggering-further-workflow-runs I’ll try one of the workarounds and try triggering it again to make sure it runs CI too.

mudge commented 1 year ago

Closing in favour of https://github.com/mudge/re2/pull/113 as that PR was raised automatically.