-
Notifications
You must be signed in to change notification settings - Fork 47
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
GzProtobuf: Do not require version 3 do support Protobuf 4.23.2 (23.2) #346
Conversation
Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
It does not seem a failure related to this PR. |
Codecov Report
@@ Coverage Diff @@
## ign-msgs5 #346 +/- ##
==========================================
Coverage 97.12% 97.12%
==========================================
Files 9 9
Lines 905 905
==========================================
Hits 879 879
Misses 26 26 |
I added some cross-compilation related changes, but they are failing:
So probably better to remove them for now, and just keep them in https://github.com/traversaro/ign-msgs/tree/protobuf-22-cross-compilation for future reference. |
Ok, the PR is ready for review now. Note that it could be helpful soon also in the context of homebrew, see Homebrew/homebrew-core#131405 . |
osrf/homebrew-simulation#2274 needs this since this version of protobuf has landed in homebrew-core |
Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
🦟 Bug fix
Fixes build of ign-msgs/gz-msgs with a recent version of Protobuf (such as 23.2).
Summary
TL;DR:
Requiring protobuf 3 in
find_package(GzProtobuf)
creates linking error when using recent protobuf builds. As no supported distribution is actually installing protobuf 2, I think we can just stop passing any required version tofind_package(GzProtobuf)
Long explanation
The new version of protobuf did some big changes:
This two combined changes broke compilation of gz-msgs with recent protobufs. In particular, the change in the major number reported by CMake means that the call:
in https://github.com/gazebosim/gz-cmake/blob/gz-cmake3/cmake/FindGzProtobuf.cmake#L29 if
GzProtobuf_FIND_VERSION
is 3, fails, asprotobuf-config.cmake
file installed by Protobuf fails if the requested major version is not the one installed (see https://github.com/protocolbuffers/protobuf/blob/v23.2/cmake/protobuf-config-version.cmake.in#L21). After that checks fails, the code falls back to use theFindProtobuf.cmake
module installed by CMake:the
FindProtobuf.cmake
instead just check if the found version is greater than the requested one (see ), and so is successful even ifGzProtobuf_FIND_VERSION
is 3 and the found protobuf version is 22.3.0 .In general, using
FindProtobuf.cmake
in place ofprotobuf-config.cmake
would be ok, but not since now protobuf links abseil: theFindProtobuf.cmake
file is not aware that abseil should be linked, whileprotobuf-config.cmake
contains all the necessary information. For this reason, before this PR linking gz-msgs against a recent protobuf results in a linking error like:To fix this error, I just propose to stop passing 3 as required version to
find_package(GzProtobuf)
.Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.