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

FindIgnProtobuf could be more specific when libprotoc-dev is missing #9

Open
osrf-migration opened this issue Dec 6, 2017 · 11 comments
Labels
enhancement New feature or request minor

Comments

@osrf-migration
Copy link

Original report (archived issue) by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).


While cleaning up a docker image based on 16.04 I had installed libprotobuf-dev and protobuf-compiler but accidentally removed libprotoc-dev.

The output at generate time is

-- Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so (Required is at least version "2.3.0") 
-- Looking for Protobuf - found

but the output at build time is

-- Build files have been written to: /workspace/ws_ign/build_isolated/ign_msgs
==> '. /workspace/ws_ign/build_isolated/ign_msgs/cmake__build.sh && /usr/bin/make -j8 -l8' in '/workspace/ws_ign/build_isolated/ign_msgs'
src/CMakeFiles/ign_msgs_gen.dir/build.make:120: *** target pattern contains no '%'.  Stop.
CMakeFiles/Makefile2:1186: recipe for target 'src/CMakeFiles/ign_msgs_gen.dir/all' failed

Line 120 in build.make is src/ign_msgs_gen: protobuf::libprotoc-NOTFOUND

I'd like the the generate step to error with "libprotoc not found". How could that be accomplished? Does it warrant it's own find module? Or would it fit better as a COMPONENT of FindIgnProtobuf?

@osrf-migration
Copy link
Author

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).


My first instinct was to say that we should have the cmake configuration throw a fatal error if libprotoc is missing, but I suppose libprotoc is only needed by projects that are generating their own protobuf message types. If a project is just trying to send/receive messages, they would just need libprotobuf and not libprotoc.

One tricky detail here is that we're leveraging the Google-provided protobuf config-files and/or find-modules which search for libprotoc, libprotobuf, and protoc all at the same time, so creating separate ign-cmake find-modules for libprotoc and libprotobuf individually would be largely redundant.

I like the idea of giving FindIgnProtobuf some spoof components. The role of the IgnProtobuf components in that case would just be to check whether libprotoc, libprotobuf, and protoc were each found by Google's config-file or find-module, and then if one of those required "components" is missing, IgnProtobuf will report a failure.

To do this, we'll need to move forward on pull request #2.

@osrf-migration
Copy link
Author

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).


See pull request #81 for a fix.

@osrf-migration osrf-migration added minor enhancement New feature or request labels Apr 15, 2020
@Ryanf55
Copy link

Ryanf55 commented Jul 7, 2023

I took a look at this, can we just remove the FindGzProtobuf.cmake entirely?

Since CMake 3.9, there are numerous improvements to the FindProtobuf.cmake. I'll submit a PR to try to use that instead.

@mjcarroll
Copy link
Contributor

I took a look at this, can we just remove the FindGzProtobuf.cmake entirely?

I think that it probably makes sense. I noticed recently that our implementation misses some of those improvements. This would be a good opportunity to do that.

@Ryanf55
Copy link

Ryanf55 commented Jul 7, 2023

Sounds good.

Relates to my comment here; protocolbuffers/protobuf#1931 (comment)

I'm just trying to compile gazebo from source on Ubuntu 23, not boil the ocean, so I'll use CMake's module rather than try to get a config version merged into protobuf.

@scpeters
Copy link
Member

I took a look at this, can we just remove the FindGzProtobuf.cmake entirely?

Since CMake 3.9, there are numerous improvements to the FindProtobuf.cmake. I'll submit a PR to try to use that instead.

Thank you for submitting these pull requests!

As noted by @traversaro in #363 (comment):

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 .

This is confirmed by the CI failure on macOS in gazebosim/gz-msgs#360 (comment)

Unfortunately, I think cmake's FindProtobuf module is not ready for us to make this change. As noted in #363 (comment), I think our current FindGzProtobuf module is the best option for now.

@Ryanf55
Copy link

Ryanf55 commented Jul 19, 2023

Thanks, I'll let this simmer. If this is fixed in CMake 3.28, could this be re-considered?

@scpeters
Copy link
Member

Thanks, I'll let this simmer. If this is fixed in CMake 3.28, could this be re-considered?

if this is fixed in cmake upstream, then it will be most straightforward to revisit when that cmake version is available in the supported platforms of the target branch

@parona-source
Copy link

parona-source commented Aug 22, 2023

Protobuf installs config mode cmake files https://cmake.org/cmake/help/latest/command/find_package.html#search-modes using them would sort out abseil related issues as it would add the linked libraries to the targets.

Got here after looking at protobuf breakage in ignition-msgs on Gentoo.

Some context
https://bugs.gentoo.org/909081
https://bugs.gentoo.org/912792

@traversaro
Copy link
Contributor

Protobuf installs config mode cmake files https://cmake.org/cmake/help/latest/command/find_package.html#search-modes using them would sort out abseil related issues as it would add the linked libraries to the targets.

Protobuf packaged for all Debian-based distro before 2023 (at least) including Ubuntu 20.04 and 22.04 is compiled via autotools, that does not install config mode cmake files (see for example https://packages.ubuntu.com/jammy/amd64/libprotobuf-dev/filelist). So to keep compatibility with Ubuntu 20.04 and 22.04 we need to support at least as a fallback support for finding protobuf via the FindProtobuf.cmake shipped with CMake.

@traversaro
Copy link
Contributor

Got here after looking at protobuf breakage in ignition-msgs on Gentoo.

Some context https://bugs.gentoo.org/909081 https://bugs.gentoo.org/912792

If I am not wrong, this should be fixed by gazebosim/gz-msgs#346 (we had the same problem in conda-forge). Probably we need either a new release of ign-msgs5 or that you backport the patch in gazebosim/gz-msgs#346 ? See also the other PRs linked in gazebosim/gz-msgs#346 for other similar fixes in ignition/gazebo libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

No branches or pull requests

6 participants