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

Use CMake's protobuf module #360

Closed

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Jul 7, 2023

Relates to gazebosim/gz-cmake#9

Summary

This is removing the need for the custom find script for protobuf.

Test it

Compile on CI

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@Ryanf55 Ryanf55 requested a review from caguero as a code owner July 7, 2023 22:36
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 7, 2023
@Ryanf55 Ryanf55 changed the title Switch to upstream cmake's protobuf find module Use CMake's protobuf module Jul 7, 2023
@@ -1,6 +1,7 @@
##################################################
# Build a custom protoc plugin
gz_add_executable(gz_msgs_gen Generator.cc generator_main.cc)
find_package(Protobuf REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this should be needed, as the top-level CMakeLists file should already be finding protobuf, correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That what I would have thought, but it failed build on my system. I didn't find the call to add_subdirectory(src) so I'm a bit lost how the CMakeLists is working in the first place.

@scpeters
Copy link
Member

as noted by @traversaro in gazebosim/gz-cmake#363 (comment)

Unfortunatly at the moment the FindProtobuf.cmake shipped with CMake does not work correctly with protobuf >= 23.2

This is the cause of the macOS CI failure in this job:

CMake Warning at /usr/local/Cellar/cmake/3.26.4/share/cmake/Modules/FindProtobuf.cmake:524 (message):
  Protobuf compiler version 23.4 doesn't match library version 4.23.4
Call Stack (most recent call first):
  CMakeLists.txt:85 (find_package)


-- Found Protobuf: /usr/local/lib/libprotobuf.dylib (found version "4.23.4") 
...
[  2%] Linking CXX executable ../bin/gz_msgs_gen
Undefined symbols for architecture x86_64:
  "absl::lts_20230125::hash_internal::MixingHashState::LowLevelHashImpl(unsigned char const*, unsigned long)", referenced from:
      absl::lts_20230125::hash_internal::MixingHashState::combine_contiguous(absl::lts_20230125::hash_internal::MixingHashState, unsigned char const*, unsigned long) in Generator.cc.o
...

@Ryanf55
Copy link
Author

Ryanf55 commented Jul 19, 2023

Closing for now to reduce the noise. It can be revisited when CMake+Protobuf work out of the box nicely again.

@Ryanf55 Ryanf55 closed this Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants