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.

Sign up for GitHub

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

Jump to bottom

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

Open
stanhu opened this issue Jul 6, 2023 · 5 comments
Open

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

stanhu opened this issue Jul 6, 2023 · 5 comments
Assignees
@apolcyn
Labels
kind/enhancement lang/ruby priority/P2

Comments

@stanhu
Copy link
Contributor

stanhu commented Jul 6, 2023

Currently the grpc gem cannot be compiled if:

  1. A recent version of re2 is installed with a custom prefix.
  2. The Ruby interpreter is compiled with CPPFLAGS with that prefix.

For example, we have:

  1. re2 2023-03-01 compiled with a prefix of /opt/gitlab/embedded.
  2. The Ruby interpreter is built with CPPFLAGS with -I/opt/gitlab/embedded/include.

When compiling the grpc gem, the grpc-core Makefile will attempt to include /opt/gitlab/embedded/include/re2/re2.h instead of the local third_party/re2/re2.h file, which uses re2 2022-04-01. The method definitions are different from re2 2023-03-01, so the compilation fails.

As discussed in #32580 (comment), the resulting behavior of extconf.rb changed over grpc versions:

  1. Prior to v1.48.0, the Ruby build process did not include any external CPPFLAGS while building grpc-core.
  2. Starting with v1.48.0, Make the gem build on TruffleRuby #27660 passed along ENV['CPPFLAGS'] while building grpc-core.
  3. v1.56.0 attempted to fix this via [ruby] fix re2 compilation when older system version installed #32580 by passing in RbConfig::CONFIG['CPPFLAGS'] instead of ENV['CPPFLAGS']. However, I overlooked that the Ruby compiler flags may also have CPPFLAGS that give precedence to the newer re2 path:
irb(main):001:0> RbConfig::CONFIG['CPPFLAGS']
=> "-I/opt/gitlab/embedded/include -O3 -D_FORTIFY_SOURCE=2 -fstack-protector -O3 -g -pipe -fno-omit-frame-pointer -I/opt/gitlab/embedded/include  "

The key problem here is that no matter what flags are supplied by the Ruby extconf.rb, I believe grpc-core should always give precedence to the the third_party directory and ensure re2 headers are included during the build process.

I attempted to do this in 37a6ff0, but according to #32580 (comment) building with the Makefile is deprecated.

Describe the solution you'd like

I think we have a few options:

  1. Reconsider the patch introduced in 37a6ff0.
  2. Adapt the Ruby C extension to use cmake instead. (?)

What do you think @veblush @eregon?
@stanhu stanhu mentioned this issue Jul 6, 2023
Closed
stanhu added a commit to stanhu/grpc that referenced this issue Jul 6, 2023
@stanhu
Fix re2 compilation when older system version installed
4e6f384 
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.

Relates to grpc#33615
stanhu added a commit to stanhu/grpc that referenced this issue Jul 6, 2023
@stanhu
[ruby] Fix re2 compilation when different system version installed
e8277b4 
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 Ruby interpreter has CPPFLAGS to include the prefix, such as
`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.

Relates to grpc#33615
stanhu added a commit to stanhu/grpc that referenced this issue Jul 6, 2023
@stanhu
[ruby] Fix re2 compilation when different system version installed
c688ba8 
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 Ruby interpreter has CPPFLAGS to include the prefix, such as
`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.

Relates to grpc#33615
@stanhu
Copy link
Contributor Author

stanhu commented Jul 6, 2023 •
edited

For now, I submitted #33616. It seems to me there should be a separate discussion about moving away from Makefile.

stanhu added a commit to stanhu/grpc that referenced this issue Jul 6, 2023
@stanhu
[ruby] Fix re2 compilation when different system version installed
81a265a 
re2 previously failed to compile if:

1. A different and incompatible `re2` version is installed with a
non-standard system prefix, such as `/opt/local`.
2. The Ruby interpreter has CPPFLAGS to include the prefix, such as
`CPPFLAGS=-I/opt/local/include`.

Running `make` would result in method 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.

Relates to grpc#33615
@knu
Copy link

knu commented Jul 8, 2023

The same goes for abseil. I always need to run brew uninstall re2 abseil protobuf-c protobuf every time I install the grpc gem.

stanhu added a commit to stanhu/grpc that referenced this issue Jul 10, 2023
@stanhu
[ruby] Fix re2 and abseil builds when different system versions insta…
20b169c 
…lled

re2 or abseil previously failed to compile if:

1. A different and incompatible version is installed with a
non-standard system prefix, such as `/opt/local`.
2. The Ruby interpreter has CPPFLAGS to include the prefix, such as
`CPPFLAGS=-I/opt/local/include`.

Running `make` would result in method 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 and abseil directories in the include path.

Relates to grpc#33615
@stanhu
Copy link
Contributor Author

stanhu commented Jul 10, 2023

@knu Ah, thanks. I was debating whether to update abseil as well. Can you verify that #33616 works for you?

@stanhu
Copy link
Contributor Author

stanhu commented Aug 26, 2023

This might be fixed by #33803, but I'd have to verify.

@stanhu
Copy link
Contributor Author

stanhu commented Aug 26, 2023

Unfortunately, the build still fails with v1.58.0.pre1.

maxlazio pushed a commit to gitlabhq/omnibus-gitlab that referenced this issue Sep 15, 2023
@stanhu
Remove libre2 from build
607ec5f 
With the update to re2 gem v2.0 in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131273, we no
longer need to build and ship libre2. The gem handles the building and
compiling of all dependencies (libre2 and abseil-cpp), and it also
ships with precompiled gems.

This unblocks upgrades to the `grpc` gem since
grpc/grpc#33615 no longer applies.

Changelog: changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@apolcyn apolcyn

Labels
kind/enhancement lang/ruby priority/P2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@knu @stanhu @apolcyn