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

Support find-protobuf using standard CMake #363

Closed
wants to merge 1 commit into from

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Jul 7, 2023

Summary

This removes the custom FindGzProtobuf since one is already supplied by CMake and we have new enough CMake available to use it. Relates to #9 (comment)

A few more PR's are going to be pushed up too.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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>
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jul 7, 2023
@traversaro
Copy link
Contributor

traversaro commented Jul 7, 2023

Unfortunatly at the moment the FindProtobuf.cmake shipped with CMake does not work correctly with protobuf >= 23.2, see gazebosim/gz-msgs#346 and https://gitlab.kitware.com/cmake/cmake/-/issues/24321 . If FindProtobuf.cmake could be used as it is, I guess we can't just remove the module from an already publish released of gz-cmake .

@scpeters
Copy link
Member

scpeters commented Jul 7, 2023

Unfortunatly at the moment the FindProtobuf.cmake shipped with CMake does not work correctly with protobuf >= 23.2, see gazebosim/gz-msgs#346 and https://gitlab.kitware.com/cmake/cmake/-/issues/24321 . If FindProtobuf.cmake could be used as it is, I guess we can't just remove the module from an already publish released of gz-cmake .

I think we could consider deprecating it on the main branch, but I think the first step would be to change the packages (like gz-msgs*) that use this find module over to the find_package call you suggest instead.

@Ryanf55
Copy link
Author

Ryanf55 commented Jul 7, 2023

Unfortunatly at the moment the FindProtobuf.cmake shipped with CMake does not work correctly with protobuf >= 23.2, see gazebosim/gz-msgs#346 and https://gitlab.kitware.com/cmake/cmake/-/issues/24321 . If FindProtobuf.cmake could be used as it is, I guess we can't just remove the module from an already publish released of gz-cmake .

I think we could consider deprecating it on the main branch, but I think the first step would be to change the packages (like gz-msgs*) that use this find module over to the find_package call you suggest instead.

Sounds good. We can start with the other PR's first.

@traversaro
Copy link
Contributor

The problem that I see is that if you use find_package(Protobuf MODULE) we break the compilation against protobuf >= 23, while if you use find_package(Protobuf CONFIG) we break the compilation of autotools-compiled Protobuf (that includes the Protobuf Debian packages in almost all Ubuntu distros).

@Ryanf55
Copy link
Author

Ryanf55 commented Jul 8, 2023

The problem that I see is that if you use find_package(Protobuf MODULE) we break the compilation against protobuf >= 23, while if you use find_package(Protobuf CONFIG) we break the compilation of autotools-compiled Protobuf (that includes the Protobuf Debian packages in almost all Ubuntu distros).

Hmm, yea. Thanks, it seems you've spent a bunch of time on this.

It's unfortunate some errors are during linking, the major distros are building with autotools, the protobuf maintainers don't seem bothered they broke CMake support on major platforms, even if we patch CMake's find module today, it will only help if you have newer than CMake 3.28.

Things to figure out:

  • Which major distros in gazebo are affected?
  • Does gazebo have to support source compilation on all combinations of host OS + gazebo version + package-installed protobuf or source-compiled-protobuf, or are there combinations that can be ignored?

Ideas:

  • Request package maintainers to switch to CMake instead of autotools, mainly Ubuntu 24
  • Request CMake backport a bugfix for version handling in their find module to the CMake versions released by major distros
  • Split the difference and try some combination of find_package(Protobuf MODULE) and find_package(Protobuf CONFIG) with perhaps some custom version handling detection for gazebo until either of the above are done

@traversaro
Copy link
Contributor

Split the difference and try some combination of find_package(Protobuf MODULE) and find_package(Protobuf CONFIG) with perhaps some custom version handling detection for gazebo until either of the above are done

The problem is that at that point if you have boilerplate you may want to avoid duplicating it across gz-* repos, and that is thea reason why FindGzProtobuf.cmake exists in the first place.

@Ryanf55
Copy link
Author

Ryanf55 commented Jul 10, 2023

Split the difference and try some combination of find_package(Protobuf MODULE) and find_package(Protobuf CONFIG) with perhaps some custom version handling detection for gazebo until either of the above are done

The problem is that at that point if you have boilerplate you may want to avoid duplicating it across gz-* repos, and that is thea reason why FindGzProtobuf.cmake exists in the first place.

Would you recommend trying to modify FindGzProtobuf then to do the following?

  1. Modify FindGzProtobuf to first try to find protobuf it in CONFIG mode with protobuf's distributed config file, which will only happen if someone built and installed it from source with CMake, since most package managers today supply the autotools version
  2. Modify FindGzProtobuf to then try to find protobuf in MODULE mode with CMake's supplied version?
  3. Keep FindGzProtobuf's existing logic as fallback hopefully can be deprecated once upstream issues are resolved

@traversaro
Copy link
Contributor

traversaro commented Jul 10, 2023

Would you recommend trying to modify FindGzProtobuf then to do the following?

1. Try to find protobuf it in CONFIG mode with protobuf's distributed config file, which will only happen if someone built and installed it from source with CMake, since most package managers today supply the autotools version

2. Try to find protobuf in MODULE mode with CMake's supplied version?

3. Fall back to custom logic that hopefully can be deprecated once upstream issues are resolved

Split the difference and try some combination of find_package(Protobuf MODULE) and find_package(Protobuf CONFIG) with perhaps some custom version handling detection for gazebo until either of the above are done

The problem is that at that point if you have boilerplate you may want to avoid duplicating it across gz-* repos, and that is thea reason why FindGzProtobuf.cmake exists in the first place.

Would you recommend trying to modify FindGzProtobuf then to do the following?

1. Try to find protobuf it in CONFIG mode with protobuf's distributed config file, which will only happen if someone built and installed it from source with CMake, since most package managers today supply the autotools version

2. Try to find protobuf in MODULE mode with CMake's supplied version?

3. Fall back to custom logic that hopefully can be deprecated once upstream issues are resolved

If I am not wrong, FindGzProtobuf already does 1. (https://github.com/gazebosim/gz-cmake/blob/gz-cmake3_3.2.2/cmake/FindGzProtobuf.cmake#L29) and 2. (https://github.com/gazebosim/gz-cmake/blob/gz-cmake3_3.2.2/cmake/FindGzProtobuf.cmake#L34), while there is no custom (find) logic.

@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 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants