cartographer-project / cartographer

Cartographer is a system that provides real-time simultaneous localization and mapping (SLAM) in 2D and 3D across multiple platforms and sensor configurations.
Apache License 2.0
7.09k stars 2.25k forks source link

Added GMOCK_LIBRARY check #1769

Closed ywiyogo closed 2 years ago

ywiyogo commented 3 years ago

Added CMake check to avoid build error if GMOCK_LIBRARY not found.

MichaelGrupp commented 3 years ago

Can you explain under which circumstances you had a problem with this?

Your change is also not compatible with our continuous integration build on Ubuntu Focal, as can be seen here: https://travis-ci.org/github/cartographer-project/cartographer/builds/739915655

ywiyogo commented 3 years ago

@MichaelGrupp, I got the issue during compiling the cartographer on Raspbian OS of my RaspberryPi4 in ROS environment using catkin_make_isolated --install --use-ninja. The debug message of CMake shows that the GMock Library is not found: ${GMOCK_LIBRARY} returns GMOCK_LIBRARY-NOTFOUND. The Gtest is found in /usr/lib/arm-linux-gnueabihf/libgtest.a

I've just analyzed my system. I did sudo apt install google-mock and it returns GMOCK_LIBRARY-NOTFOUND. Just now, I found out that I run sudo apt install libgmock-dev, and it installed some libraries, and then CMake can find the GMOCK_LIBRARY.

MichaelGrupp commented 3 years ago

Thanks for the info. As of today, we depend on the google-mock package in our package.xml.

According to Debian Buster, libgmock-dev (or googletest for src) should be used instead of google-mock:

"NOTE: This is a transitional package, retained for backwards compatibility. New code should instead use either package libgmock-dev (for compiled lib) or package googletest (for lib sources)" https://packages.debian.org/buster/google-mock

On Stretch, google-mock is also not recommended but the suggested alternative is different (only googletest):

NOTE: This is a transitional package, retained for backwards compatibility. New code should use package googletest instead. https://packages.debian.org/stretch/google-mock

This is a bit confusing, especially since we want to support both right now. Interestingly, I didn't face this when doing a fresh installation on Ubuntu 20 with the same revision of Cartographer.

Poking @wohe who was involved in a similar discussion around libgmock-dev here: https://github.com/cartographer-project/cartographer/pull/1705#discussion_r455128145

MichaelGrupp commented 3 years ago

Note: in our CI scripts for the non-ROS part, we have this:

https://github.com/cartographer-project/cartographer/blob/de76ed9fdc1b32e5080c1c4c87730e1097020135/scripts/install_debs_cmake.sh#L42-L44

TODO: If it is mandatory to install libgmock-dev, but only on Buster/Focal, we should note this down in the user docs if it's not there yet.

ywiyogo commented 3 years ago

Thanks for the info too @MichaelGrupp. No wonder this issue is so confusing.

On Stretch, google-mock is also not recommended but the suggested alternative is different (only googletest):

That was exactly the issue on my RaspPi4. If CMake cannot find the variable ${GMOCK_LIBRARY}, it throws a build error on the list(append..) line because of the GMOCK_LIBRARY-NOTFOUND..

MichaelGrupp commented 3 years ago

Update: this looks like an issue in how rosdep resolves the key google-mock. It should use the correct package on these distros. I opened a pull request in rosdistro, let's wait how this goes: https://github.com/ros/rosdistro/pull/27195

ywiyogo commented 3 years ago

@MichaelGrupp What do you think about this workaround?

    if(GMOCK_LIBRARY_FOUND)
      list(APPEND GMOCK_LIBRARIES ${GMOCK_LIBRARY})
    endif()
    list(APPEND GMOCK_LIBRARIES ${GTEST_LIBRARY})

I just checked again, and the issue is only for GMOCK_LIBRARY.

bochen87 commented 2 years ago

since https://github.com/ros/rosdistro/pull/27195 was merged, can we close this PR?

ywiyogo commented 2 years ago

since ros/rosdistro#27195 was merged, can we close this PR?

yes please