search

gazebosim / gz-cmake

A set of CMake modules that are used by the C++-based Gazebo projects.
https://gazebosim.org/libs/cmake
Apache License 2.0
24 stars 30 forks source link

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

Open osrf-migration opened 6 years ago

osrf-migration commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

See pull request #81 for a fix.

Ryanf55 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sounds good.

Relates to my comment here; https://github.com/protocolbuffers/protobuf/issues/1931#issuecomment-1625754552

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 commented 1 year ago

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 https://github.com/gazebosim/gz-cmake/pull/363#issuecomment-1625794950:

Unfortunatly at the moment the FindProtobuf.cmake shipped with CMake does not work correctly with protobuf >= 23.2, see https://github.com/gazebosim/gz-msgs/pull/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 https://github.com/gazebosim/gz-msgs/pull/360#issuecomment-1638701721

Unfortunately, I think cmake's FindProtobuf module is not ready for us to make this change. As noted in https://github.com/gazebosim/gz-cmake/pull/363#issuecomment-1629572891, I think our current FindGzProtobuf module is the best option for now.

Ryanf55 commented 1 year ago

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

scpeters commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/gazebosim/gz-msgs/pull/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 https://github.com/gazebosim/gz-msgs/pull/346 ? See also the other PRs linked in https://github.com/gazebosim/gz-msgs/pull/346 for other similar fixes in ignition/gazebo libraries.