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.

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

Provide proper shared library versioning and cmake support #304

Closed
essandess opened this issue Mar 25, 2021 · 13 comments
Closed

Provide proper shared library versioning and cmake support #304

essandess opened this issue Mar 25, 2021 · 13 comments

Comments

@essandess
Copy link

re2 is in the critical path of several large projects that rely on cmake builds of grpc, apache-arrow, and others, including several tensorflow-related and other Google projects like magenta. See:

Yet the support of a basic build chain for re2 in common environments (macOS, Linux) isn't yet supported, and done so "unenthusiastically", forcing hacky backflips like this, this, and this just to provide shared library versioning.

Would you please provide support of shared library versioning and if not enthusiastic, then basic cmake support that gets the versioning correct?

Other builds rely on all these, and these issues must be hacked in or ignored now.

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

Having written a Findre2.cmake file for gRPC as per #274, I'm not keen to write a re2-config.cmake file for RE2 itself, so... I imagine the way forward is to mirror SONAME and re2.pc handling in CMake. (And substitute SONAME into re2.pc, I gather.) Could you please confirm that these actions will fully resolve the issues as you see them?

@essandess
Copy link
Author

Having written a Findre2.cmake file for gRPC as per #274, I'm not keen to write a re2-config.cmake file for RE2 itself, so... I imagine the way forward is to mirror SONAME and re2.pc handling in CMake. (And substitute SONAME into re2.pc, I gather.) Could you please confirm that these actions will fully resolve the issues as you see them?

There are multiple issues:

  1. The Makefile build produces different results and versioning than the cmake build; see re2: change to cmake build to support gprc, which currently needs a cmake version of re2 to build. Try to make it work like the previous autotools build worked. macports/macports-ports#9836 (comment)
  2. The versioning of both Makefile and cmake builds is wrong; see re2: change to cmake build to support gprc, which currently needs a cmake version of re2 to build. Try to make it work like the previous autotools build worked. macports/macports-ports#9836 (comment)
  3. Downstream projects don’t work with the Makefile build because of the cmake file for re2 in grpc; see https://issues.apache.org/jira/browse/ARROW-11888?focusedCommentId=17300689&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17300689, https://formulae.brew.sh/formula/re2

Mirroring SONAME addresses the first issue.

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

Having now read about "compatibility version" versus "current version", I think that the following changes are needed in order to set things straight on macOS (and Linux) for both builds:

  • In Makefile, pass -compatibility_version $(SONAME) and -current_version $(SONAME).0.0 when linking the shared library on macOS.
  • In CMakeLists.txt, call set_target_properties(re2 PROPERTIES SOVERSION ${SONAME} VERSION ${SONAME}.0.0).

copybara-service bot pushed a commit that referenced this issue Mar 26, 2021
For #304.

Change-Id: I74e2720ecad9a0b22e8cd73dbffe2e02599827c5
Reviewed-on: https://code-review.googlesource.com/c/re2/+/58450
Reviewed-by: Paul Wankadia <[email protected]>
@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

Could you please verify that commit 13ebb37 works as intended, @essandess?

(Ping, @mtorromeo and @foutrelis! Arch Linux was cited in the original comment, so these changes are likely to be RTYI.)

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

Thanks to macports/macports-ports#9836, I learned about the cute trick that Homebrew uses: running make common-install to install re2.pc separately from invoking CMake to build and install everything else.

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

  1. The Makefile build produces different results and versioning than the cmake build;
  1. The versioning of both Makefile and cmake builds is wrong;

I believe that both issues have been resolved; commit 13ebb37 will be in release 2021-04-01, which I will cut next Thursday.

  1. Downstream projects don’t work with the Makefile build because of the cmake file for re2 in grpc;

Fixing that should just be a case of conditionally defining re2::re2 as @kou suggested, which I will do for grpc/grpc#25434.

(Ping, @veblush and @donnadionne!)

@essandess
Copy link
Author

@junyer Thank you very much for addressing these issues so promptly.

Given all the moving pieces here, and the fact that MacPorts just merged in cmake builds of grpc (macports/macports-ports#9729) and re2 (macports/macports-ports#9836), my plan is to update and modify the MacPorts builds of these projects when you all issue new releases, at the same time hoping this doesn't cause any unforeseen build issues in other downstream projects.

The test of success has been building apache-arrow without hitting the cmake re2::re2 errors.

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

Great to hear. :)

@junyer
Copy link
Contributor

junyer commented Mar 26, 2021

(Ping, @mtorromeo and @foutrelis! Arch Linux was cited in the original comment, so these changes are likely to be RTYI.)

Oh, and @eli-schwartz as well. :)

@eli-schwartz
Copy link

eli-schwartz commented Mar 31, 2021

Well I did originally suggest the use of pkg_check_modules with the keywords:

  • IMPORTED_TARGET, does all the magic of setting up add_library() bits for you, and is idempotent already (requires cmake 3.6)
  • GLOBAL, ensures if pkg_check_modules is run from a subdir/CMakeLists.txt the target is available from the parent dir, probably not necessary here (requires cmake 3.13)

Given grpc currently supports cmake 3.5, the hack you added in grpc/grpc#25821 is necessary unless you vendor the latest version of FindPkgConfig.cmake (which I did in another project, because cmake scares me enough already and I want to stick to drawing inside the lines if at all possible, and that means backporting official cmake distribution files to use their documented functionality).

There is a TODO there to that effect... so basically the problem here is rediscovering the problems solved in later versions of cmake.

@eli-schwartz
Copy link

Having now read about "compatibility version" versus "current version", I think that the following changes are needed in order to set things straight on macOS (and Linux) for both builds:

  • In Makefile, pass -compatibility_version $(SONAME) and -current_version $(SONAME).0.0 when linking the shared library on macOS.

  • In CMakeLists.txt, call set_target_properties(re2 PROPERTIES SOVERSION ${SONAME} VERSION ${SONAME}.0.0).

Arch Linux ended up sticking with the Makefile build, whose Darwin changes don't affect us. No clue on that count. The cmake properties look correct to me based on eyeballing, but haven't tested.

@junyer
Copy link
Contributor

junyer commented Mar 31, 2021

Thanks for taking a look! I'm not sure whether gRPC has tied their minimum CMake version to Ubuntu LTS releases, but Xenial has 3.5, Bionic has 3.10 and Focal has 3.16, so we might be waiting a while for GLOBAL. :(

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this issue Apr 26, 2022
https://build.opensuse.org/request/show/972798
by user gmbr3 + dimstar_suse
- Disable tests on ZSystems and RISCV
- Switch build to CMake, otherwise CMake config is not installed.
  Required for Apache ORC and arrow, and google-or-tools.
  (google/re2#304)
- Run some real tests via CTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants