ros-industrial / ros_industrial_cmake_boilerplate

Other
16 stars 14 forks source link

don't bundle gtest without gmock & **please** don't install a `gtest/...` header #54

Closed v4hn closed 2 years ago

v4hn commented 2 years ago

I just overhauled my MoveIt-and-associated workspace, which includes stomp_ros, which now depends on stomp, which depends on ros_industrial_boilerplate. Now, the last of these bundles gtest and actually installs a gtest/ include folder in the standard workspace includes. I would argue this is a bug already because gtest

Because my system version of googletest is 1.10.0, this combination produces incompatibilities:

/usr/src/googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:460: error: no template named 'IndexSequence'; did you mean 'std::index_sequence'? In file included from moveit_task_constructor/core/test/test_cost_queue.cpp:6: In file included from /usr/src/googletest/googlemock/include/gmock/gmock-matchers.h:57: /usr/src/googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:460:37: error: no template named 'IndexSequence'; did you mean 'std::index_sequence'?

I'm not sure which fix you prefer/deem feasible, but please address this @Levi-Armstrong .

Levi-Armstrong commented 2 years ago

What OS and version are you running on? If GTest exists on the OS it should not be installing GTest in the workspace. It first calls find_package for GTest and if it does not exist it then installs it.

should definitely not overshadow gtest for every package using the workspace - so includes should be installed to a different include path

I see no reason why this is an issue. This is not an issue with any other package in the workspace which may or may not have a system-wide version. Why is this an issue here?

The package does not bundle gmock as well, so packages that include gmock will include the system installation of gmock, but the ros_industrial version of gtest

This should be an easy fix. In the GTest CMakeLists.txt file, there is an option for GMock if you enable this does it resolve the issue?

v4hn commented 2 years ago

What OS are you running on?

Lunar-Linux. Granted, it's a niche system.

If GTest exists on the OS it should not be installing GTest in the workspace. It first calls find_package for GTest and if it does not exist it then installs it.

True, it can't find GTest on my system like that. But I just tested on Ubuntu bionic with googletest and libgtest-dev installed and it also fails there? I guess there might be a reason why catkin does not use it? Or is this just a consistent problem on my setups?

should definitely not overshadow gtest for every package using the workspace - so includes should be installed to a different include path

I see no reason why this is the issue. This is not an issue with any other package in the workspace which may or may not have a system-wide version. Why is this an issue here?

If the overlay works correctly it's not really an issue because standard linux systems should provide gtest already. Either way, it is an issue for other packages as well. To give an example, OMPL migrated to versioned header installs two years ago to mitigate the same problem. And gtest is arguably more basic. :-)

The package does not bundle gmock as well, so packages that include gmock will include the system installation of gmock, but the ros_industrial version of gtest

This should be an easy fix. In the GTest CMakeLists.txt file, there is an option for GMock if you enable this does it resolve the issue?

That works and fixes the original bug I hit. Please change this upstream. :+1:

It still does not solve why/how the check does not work and as a result this injects itself into the workspace unconditionally.

Thanks for your quick response! :+1:

Levi-Armstrong commented 2 years ago

True, it can't find GTest on my system like that. But I just tested on Ubuntu bionic with googletest and libgtest-dev installed and it also fails there? I guess there might be a reason why catkin does not use it? Or is this just a consistent problem on my setups?

I have had it not find GTest on older versions of Ubuntu but on 20.04 it is able to find it. I think the older versions only provided the headers and you had to build the libraries wherein 20.04 they started providing the shared libraries.

If the overlay works correctly it's not really an issue because standard linux systems should provide gtest already. Either way, it is an issue for other packages as well. To give an example, OMPL migrated to versioned header installs two years ago to mitigate the same problem. And gtest is arguably more basic. :-)

I am interested in improving this; how do you recommend resolving this here?

Levi-Armstrong commented 2 years ago

@v4hn I merged in a PR which enable GMOCK

v4hn commented 2 years ago

I have had it not find GTest on older versions of Ubuntu but on 20.04 it is able to find it. I think the older versions only provided the headers and you had to build the libraries wherein 20.04 they started providing the shared libraries.

Ah yes, makes sense. cmake's FindGTest.cmake can require a pre-built library. But then I'm still wondering. Why would you even force an older version of googletest on ubuntu 20.04? Where there big API breaks between 1.8 and 1.10?

I am interested in improving this; how do you recommend resolving this here?

Usually you would rename either the whole package or move the header install folder into a custom subfolder, e.g., gtest_ros_industrial. That way the headers will only be picked up when the package is explicitly find_packaged.

I don't have enough time to play this through with workspaces, but gtest uses GNUInstallDirs. So maybe it's enough to add -DCMAKE_INSTALL_INCLUDEDIR=include/gtest_ros_industrial to the build instructions. But you will need to check whether this plays out correctly in your workspaces.

Levi-Armstrong commented 2 years ago

But then I'm still wondering. Why would you even force an older version of googletest on ubuntu 20.04? Where there big API breaks between 1.8 and 1.10?

This package was created when developing on 18.04 which I assume was the version then. It was never updated when we transition to 20.04. I will update to 1.10.

Levi-Armstrong commented 2 years ago

The gtest tag has been updated #56