[ruby] fix re2 compilation when older system version installed #32580

stanhu · 2023-03-09T09:26:36Z

re2 previously failed to compile if:

An old re2 version is installed with a non-standard system prefix, such as /opt/local.
The environment variable is set: CPPFLAGS=-I/opt/local/include.

Running make would result in function prototype mismatches because the Makefile would previously attempt to use the headers from /opt/local/include/re2 before the third_party/re2/re2 directory.

#27660 caused CPPFLAGS to inherit from the environment, but this can cause the Makefile to use external include files for re2 and other libraries if -I flags are defined.

This commit reverts to the original behavior of only using RbConfig::CONFIG values to avoid using the wrong headers.

re2 previously failed to compile if: 1. An old `re2` version is installed with a non-standard system prefix, such as `/opt/local`. 2. The environment variable is set: `CPPFLAGS=-I/opt/local/include`. Running `make` would result in function prototype mismatches because the Makefile would previously attempt to use the headers from `/opt/local/include/re2` before the `third_party/re2/re2` directory. To ensure the current directories receive priority, include the relevant re2 directories in the include path.

stanhu · 2023-03-09T10:11:13Z

Something changed between v1.42.0 and v1.50.0. Here's the search order before and after. Notice where /opt/re2/include shows up:

Search order in v1.42.0

g++ -Ithird_party/boringssl-with-bazel/src/include -Ithird_party/address_sorting/include -Ithird_party/cares -Ithird_party/cares/cares -DGPR_BACKWARDS_COMPATIBILITY_MODE -DGRPC_XDS_USER_AGENT_NAME_SUFFIX="\"RUBY\""  -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX="\"1.42.0\""  -g -Wall -Wextra -DOSATOMIC_USE_INLINED=1 -Ithird_party/abseil-cpp -Ithird_party/re2 -Ithird_party/upb -Isrc/core/ext/upb-generated -Isrc/core/ext/upbdefs-generated -Ithird_party/xxhash -O2 -Wframe-larger-than=16384 -fPIC -I. -Iinclude -I/tmp/grpc/gens -DNDEBUG -DINSTALL_PREFIX=\"/usr/local\"   -Ithird_party/zlib   -I/opt/re2/include -std=c++11   -MMD -MF /tmp/grpc/objs/opt/third_party/re2/re2/prog.dep -c -o /tmp/grpc/objs/opt/third_party/re2/re2/prog.o third_party/re2/re2/prog.cc
mkdir -p `dirname /tmp/grpc/objs/opt/third_party/re2/re2/regexp.o`
ignoring duplicate directory "/usr/include/x86_64-linux-gnu/c++/10"
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/10/include-fixed"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/10/../../../../x86_64-linux-gnu/include"
ignoring nonexistent directory "/tmp/grpc/gens"
#include "..." search starts here:
#include <...> search starts here:
 third_party/boringssl-with-bazel/src/include
 third_party/address_sorting/include
 third_party/cares
 third_party/cares/cares
 third_party/abseil-cpp
 third_party/re2
 third_party/upb
 src/core/ext/upb-generated
 src/core/ext/upbdefs-generated
 third_party/xxhash
 .
 include
 third_party/zlib
 /opt/re2/include
 /usr/include/c++/10
 /usr/include/x86_64-linux-gnu/c++/10
 /usr/include/c++/10/backward
 /usr/lib/gcc/x86_64-linux-gnu/10/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include

Search order in v1.52.0

 /usr/lib/gcc/x86_64-linux-gnu/10/cc1plus -quiet -v -I third_party/boringssl-with-bazel/src/include -I third_party/address_sorting/include -I third_party/cares/cares/include -I third_party/cares -I third_party/cares/cares -I /opt/re2/include -I third_party/abseil-cpp -I third_party/re2 -I third_party/upb -I src/core/ext/upb-generated -I src/core/ext/upbdefs-generated -I third_party/xxhash -I . -I include -I /tmp/grpc/gens -I third_party/zlib -I /opt/re2/include -imultiarch x86_64-linux-gnu -MMD /tmp/grpc/objs/opt/third_party/re2/re2/prefilter.d -MF /tmp/grpc/objs/opt/third_party/re2/re2/prefilter.dep -MQ /tmp/grpc/objs/opt/third_party/re2/re2/prefilter.o -D_GNU_SOURCE -D GPR_BACKWARDS_COMPATIBILITY_MODE -D GRPC_XDS_USER_AGENT_NAME_SUFFIX="RUBY" -D GRPC_XDS_USER_AGENT_VERSION_SUFFIX="1.52.0" -D OSATOMIC_USE_INLINED=1 -D NDEBUG -D INSTALL_PREFIX="/usr/local" third_party/re2/re2/prefilter.cc -quiet -dumpbase prefilter.cc -mtune=generic -march=x86-64 -auxbase-strip /tmp/grpc/objs/opt/third_party/re2/re2/prefilter.o -g -O2 -Wall -Wextra -Wframe-larger-than=16384 -std=c++14 -version -fPIC -fasynchronous-unwind-tables -o /tmp/ccrgtw4F.s
GNU C++14 (Debian 10.2.1-6) version 10.2.1 20210110 (x86_64-linux-gnu)
	compiled by GNU C version 10.2.1 20210110, GMP version 6.2.1, MPFR version 4.1.0, MPC version 1.2.0, isl version isl-0.23-GMP

GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
ignoring duplicate directory "/usr/include/x86_64-linux-gnu/c++/10"
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/10/include-fixed"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/10/../../../../x86_64-linux-gnu/include"
ignoring nonexistent directory "/tmp/grpc/gens"
ignoring duplicate directory "/opt/re2/include"
#include "..." search starts here:
#include <...> search starts here:
 third_party/boringssl-with-bazel/src/include
 third_party/address_sorting/include
 third_party/cares/cares/include
 third_party/cares
 third_party/cares/cares
 /opt/re2/include
 third_party/abseil-cpp
 third_party/re2
 third_party/upb
 src/core/ext/upb-generated
 src/core/ext/upbdefs-generated
 third_party/xxhash
 .
 include
 third_party/zlib
 /usr/include/c++/10
 /usr/include/x86_64-linux-gnu/c++/10
 /usr/include/c++/10/backward
 /usr/lib/gcc/x86_64-linux-gnu/10/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include

stanhu · 2023-03-09T10:17:48Z

I should note I'm building the Ruby C extension, so perhaps there's a change in src/ruby/ext/grpc/extconf.rb that's causing the environment's CPPFLAGS to take precedence. Perhaps 0a5d982.

stanhu · 2023-03-09T15:16:25Z

Ok, in v1.42.0, the CPPFLAGS were not appended in the system(cmd) call to compile gRPC, so any custom CPPFLAGS were not included when building grpc :

grpc/src/ruby/ext/grpc/extconf.rb

Lines 61 to 63 in 11f0048

    
           ENV['CPPFLAGS'] = '-DGPR_BACKWARDS_COMPATIBILITY_MODE' 
        
           ENV['CPPFLAGS'] += ' -DGRPC_XDS_USER_AGENT_NAME_SUFFIX="\"RUBY\"" ' 
        
           ENV['CPPFLAGS'] += ' -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX="\"1.42.0\"" '

In v1.52.0, CPPFLAGS are appended from the environment:

grpc/src/ruby/ext/grpc/extconf.rb

Lines 87 to 91 in febed51

    
           env_append 'CPPFLAGS', '-DGPR_BACKWARDS_COMPATIBILITY_MODE' 
        
           env_append 'CPPFLAGS', '-DGRPC_XDS_USER_AGENT_NAME_SUFFIX="\"RUBY\""' 
        
           require_relative '../../lib/grpc/version' 
        
           env_append 'CPPFLAGS', '-DGRPC_XDS_USER_AGENT_VERSION_SUFFIX="\"' + GRPC::VERSION + '\""'

With 0a5d982, CPPFLAGS from the environment are passed along, so a -I<directory> will be inserted before most libraries and alter the search path.

I also can't use build config build.grpc --with-cppflags='' here since that system(cmd) doesn't pass along those flags.

With this pull request, the search path now looks something like this when CPPFLAGS=-I/opt/re2/include is present. Note where /opt/re2/include lies:

#include "..." search starts here:
#include <...> search starts here:
 third_party/boringssl-with-bazel/src/include
 third_party/re2
 third_party/address_sorting/include
 third_party/cares/cares/include
 third_party/cares
 third_party/cares/cares
 /opt/re2/include
 third_party/abseil-cpp
 third_party/upb
 src/core/ext/upb-generated
 src/core/ext/upbdefs-generated
 third_party/xxhash
 .
 include
 third_party/zlib
 /usr/include/c++/10
 /usr/include/x86_64-linux-gnu/c++/10
 /usr/include/c++/10/backward
 /usr/lib/gcc/x86_64-linux-gnu/10/include
 /usr/local/include
 /usr/include/x86_64-linux-gnu
 /usr/include

Maybe we should prepend CPPFLAGS instead of appending everywhere?

/cc: @apolcyn, @eregon

stanhu · 2023-03-09T18:45:37Z

Maybe we should prepend CPPFLAGS instead of appending everywhere?

I suppose in the case of Truffleruby, it was intentional that these CPPFLAGS precede the gRPC library flags in order to get the system OpenSSL headers.

Maybe we want to make sure abseil-cpp and upb don't have the same issue?

diff --git a/Makefile b/Makefile
index 72901b111a..fa562efff5 100644
--- a/Makefile
+++ b/Makefile
@@ -534,6 +534,7 @@ CPPFLAGS := -Ithird_party/address_sorting/include $(CPPFLAGS)
 
 GRPC_ABSEIL_DEP = $(LIBDIR)/$(CONFIG)/libgrpc_abseil.a
 GRPC_ABSEIL_MERGE_LIBS = $(LIBDIR)/$(CONFIG)/libgrpc_abseil.a
+CPPFLAGS := -Ithird_party/abseil-cpp $(CPPFLAGS)
 
 # Setup re2 dependency
 
@@ -547,6 +548,7 @@ CPPFLAGS := -Ithird_party/re2 $(CPPFLAGS)
 UPB_DEP = $(LIBDIR)/$(CONFIG)/libupb.a
 UPB_MERGE_OBJS = $(LIBUPB_OBJS)
 UPB_MERGE_LIBS = $(LIBDIR)/$(CONFIG)/libupb.a
+CPPFLAGS := -Ithird_party/upb $(CPPFLAGS)
 
 # Setup boringssl dependency

But this still leaves a few other packages vulnerable to being prefixed by CPPFLAGS:

 src/core/ext/upb-generated
 src/core/ext/upbdefs-generated
 third_party/xxhash
 third_party/zlib

veblush · 2023-03-14T20:02:42Z

It's not clear to me about what the problem is and how to fix it as we deprecated Makefile already and it's there only to generate artifacts such as Ruby. So I just reassigned to Alex.

eregon · 2023-03-16T19:53:46Z

Maybe we should prepend CPPFLAGS instead of appending everywhere?

If we prepend then the user-supplied CPPFLAGS env var would override the values set in extconf.rb, is that really what we want? I think that's inherently more brittle, no? But I might misunderstand the issue.

eregon · 2023-03-16T19:57:21Z

A possibility could be instead of inherit_rbconfig 'CPPFLAGS', just ENV['CPPFLAGS'] = RbConfig::CONFIG['CPPFLAGS'] || '', i.e., ignore the CPPFLAGS env var to restore the previous behavior.

stanhu · 2023-03-16T21:15:13Z

A possibility could be instead of inherit_rbconfig 'CPPFLAGS', just ENV['CPPFLAGS'] = RbConfig::CONFIG['CPPFLAGS'] || '', i.e., ignore the CPPFLAGS env var to restore the previous behavior.

That would work. I wasn't sure if that would have worked for TruffleRuby.

This reverts commit 37a6ff0.

grpc#27660 caused `CPPFLAGS` to inherit from the environment, but this can cause the Makefile to use external include files for re2 and other libraries if `-I` flags are defined. This commit reverts to the original behavior of only using `RbConfig::CONFIG` values to avoid using the wrong headers.

stanhu · 2023-03-18T14:42:53Z

@eregon Can you review if this works for you?

eregon

Yes, this is fine and looks more reliable (doesn't depend on random stuff in the user ENV, except for LD/LDXX).

eregon · 2023-03-22T20:46:01Z

One thing though is this no longer allows overriding CC via the ENV if necessary (like it was originally). I don't know if that's needed (it's not for TruffleRuby, always using RbConfig::CONFIG is best on TruffleRuby).

This makes it possible to override the compiler and other programs.

stanhu · 2023-03-23T15:49:50Z

@eregon Oh, that's a good point. I've updated this pull request with those changes.

Ruby MRI also supports --with-cppflags and --with-cflags as part of mkmf. These values can be accessed via $CPPFLAGS and $CFLAGS. Is there a reason we don't use this as well? Does TruffleRuby also support this?

eregon · 2023-03-23T18:52:42Z

Looks good and should be compatible with the original logic.

Re $CPPFLAGS, TruffleRuby uses the same mkmf stdlib so that should work equally. There is even append_cppflags I saw. But I wouldn't change that in this PR.

The only real difference with TruffleRuby is one must respect RbConfig values for CC (otherwise there is not bitcode, and that's needed by GraalVM LLVM) and CPPFLAGS/cppflags (otherwise the ABI version is lost).

stanhu · 2023-03-23T20:48:50Z

@eregon Thanks.

@apolcyn Could you review this?

stanhu · 2023-04-10T17:53:09Z

@apolcyn Is there anything else I need to do to get this merged?

stanhu · 2023-05-04T05:30:30Z

@apolcyn Sorry to ping you again. This pull request is blocking our ability to upgrade grpc. Could you take a look?

The previous version (2016-02-01) is pretty old, and it appears that the latest grpc gem attempts to build with the system-installed re2 headers due to grpc/grpc#32580 (comment). Update to a more recent version to work around an issue building the gem. Changelog: changed

anatol · 2023-05-16T02:52:59Z

An Arch Linux bug report related to this PR https://bugs.archlinux.org/task/78379

…32580) re2 previously failed to compile if: 1. An old `re2` version is installed with a non-standard system prefix, such as `/opt/local`. 2. The environment variable is set: `CPPFLAGS=-I/opt/local/include`. Running `make` would result in function prototype mismatches because the Makefile would previously attempt to use the headers from `/opt/local/include/re2` before the `third_party/re2/re2` directory. grpc#27660 caused `CPPFLAGS` to inherit from the environment, but this can cause the Makefile to use external include files for re2 and other libraries if `-I` flags are defined. This commit reverts to the original behavior of only using `RbConfig::CONFIG` values to avoid using the wrong headers.

re2 previously failed to compile if: 1. An old `re2` version is installed with a non-standard system prefix, such as `/opt/local`. 2. The environment variable is set: `CPPFLAGS=-I/opt/local/include`. Running `make` would result in function prototype mismatches because the Makefile would previously attempt to use the headers from `/opt/local/include/re2` before the `third_party/re2/re2` directory. #27660 caused `CPPFLAGS` to inherit from the environment, but this can cause the Makefile to use external include files for re2 and other libraries if `-I` flags are defined. This commit reverts to the original behavior of only using `RbConfig::CONFIG` values to avoid using the wrong headers.

On platforms that need to compile the grpc native extension, there is a bug introduced in grpc v1.48.0 that causes the native extension to include re2 headers from `/opt/gitlab/include` instead of `vendor/re2`. To work around that problem, add `vendor/re2` in the include path. Relates to: * https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/7815 * Relates to grpc/grpc#32580 Changelog: fixed

stanhu force-pushed the sh-fix-re2-system-conflicts branch from c358aea to 01cb43c Compare March 9, 2023 09:28

grpc-bot assigned eugeneo Mar 9, 2023

stanhu force-pushed the sh-fix-re2-system-conflicts branch from 01cb43c to 37a6ff0 Compare March 9, 2023 10:08

eugeneo requested a review from veblush March 9, 2023 18:35

eugeneo removed their assignment Mar 9, 2023

grpc-bot assigned veblush Mar 9, 2023

apolcyn added kokoro:force-run lang/ruby release notes: no Indicates if PR should not be in release notes labels Mar 9, 2023

grpc-kokoro removed the kokoro:force-run label Mar 9, 2023

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Mar 9, 2023

veblush requested review from apolcyn and removed request for veblush March 14, 2023 20:02

veblush assigned apolcyn and unassigned veblush Mar 14, 2023

veblush added the area/build label Mar 14, 2023

stanhu added 2 commits March 18, 2023 07:28

Revert "Fix re2 compilation when older system version installed"

8cc4ac4

This reverts commit 37a6ff0.

stanhu force-pushed the sh-fix-re2-system-conflicts branch from 3496434 to 857897a Compare March 18, 2023 14:40

github-actions bot added the lang/Python label Mar 18, 2023

stanhu changed the title ~~Fix re2 compilation when older system version installed~~ ruby: Fix re2 compilation when older system version installed Mar 18, 2023

stanhu changed the title ~~ruby: Fix re2 compilation when older system version installed~~ ruby: fix re2 compilation when older system version installed Mar 18, 2023

eregon reviewed Mar 22, 2023

View reviewed changes

ruby: use environment variables for AR, CC, CXX, RANLIB, STRIP

135c68e

This makes it possible to override the compiler and other programs.

gnossen removed the lang/Python label Apr 5, 2023

github-actions bot added the PR title format label Apr 5, 2023

stanhu changed the title ~~ruby: fix re2 compilation when older system version installed~~ [ruby] fix re2 compilation when older system version installed Apr 5, 2023

github-actions bot removed the PR title format label Apr 5, 2023

apolcyn approved these changes May 16, 2023

View reviewed changes

apolcyn added the kokoro:force-run label May 16, 2023

grpc-kokoro removed the kokoro:force-run label May 16, 2023

stanley-cheung approved these changes May 16, 2023

View reviewed changes

apolcyn added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels May 16, 2023

apolcyn merged commit 01c87e2 into grpc:master May 16, 2023
59 of 62 checks passed

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 16, 2023

stanhu mentioned this pull request Jul 6, 2023

[ruby] Unable to build grpc gem if a more recent version of re2 is installed in include path of Ruby's CPPFLAGS #33615

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ruby] fix re2 compilation when older system version installed #32580

[ruby] fix re2 compilation when older system version installed #32580

stanhu commented Mar 9, 2023 •

edited

stanhu commented Mar 9, 2023 •

edited

stanhu commented Mar 9, 2023

stanhu commented Mar 9, 2023 •

edited

stanhu commented Mar 9, 2023

veblush commented Mar 14, 2023

eregon commented Mar 16, 2023

eregon commented Mar 16, 2023

stanhu commented Mar 16, 2023

stanhu commented Mar 18, 2023

eregon left a comment

eregon commented Mar 22, 2023 •

edited

stanhu commented Mar 23, 2023

eregon commented Mar 23, 2023

stanhu commented Mar 23, 2023

stanhu commented Apr 10, 2023

stanhu commented May 4, 2023

anatol commented May 16, 2023

[ruby] fix re2 compilation when older system version installed #32580

[ruby] fix re2 compilation when older system version installed #32580

Conversation

stanhu commented Mar 9, 2023 • edited

stanhu commented Mar 9, 2023 • edited

Search order in v1.42.0

Search order in v1.52.0

stanhu commented Mar 9, 2023

stanhu commented Mar 9, 2023 • edited

stanhu commented Mar 9, 2023

veblush commented Mar 14, 2023

eregon commented Mar 16, 2023

eregon commented Mar 16, 2023

stanhu commented Mar 16, 2023

stanhu commented Mar 18, 2023

eregon left a comment

Choose a reason for hiding this comment

eregon commented Mar 22, 2023 • edited

stanhu commented Mar 23, 2023

eregon commented Mar 23, 2023

stanhu commented Mar 23, 2023

stanhu commented Apr 10, 2023

stanhu commented May 4, 2023

anatol commented May 16, 2023

stanhu commented Mar 9, 2023 •

edited

stanhu commented Mar 9, 2023 •

edited

stanhu commented Mar 9, 2023 •

edited

eregon commented Mar 22, 2023 •

edited