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

Define GMOCK_* and GTEST_* variables in a new subproject #1101

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jul 1, 2020

This fixes the following bug occurring for a cmake project that comprises multiple catkin sub-projects:

-- Using CATKIN_ENABLE_TESTING: ON
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: /tmp/build/test_results
-- Found gtest sources under '/usr/src/googletest': gtests will be built
-- Found gmock sources under '/usr/src/googletest': gmock will be built
...
-- Using CATKIN_ENABLE_TESTING: ON
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: /tmp/build/test_results
-- Found gmock: gmock and gtests will be built
CMake Error at /opt/ros/melodic/share/catkin/cmake/assert.cmake:3 (message):

  Assertion failed: GTEST_LIBRARIES (value is '')

Call Stack (most recent call first):
  /opt/ros/melodic/share/catkin/cmake/test/gtest.cmake:185 (assert)
  /opt/ros/melodic/share/catkin/cmake/test/gtest.cmake:89 (_catkin_add_executable_with_google_test)
  /opt/ros/melodic/share/catkin/cmake/test/gtest.cmake:37 (_catkin_add_google_test)
  client/CMakeLists.txt:157 (catkin_add_gtest)

I think this issue was introduced in #1091 (definitely after release 0.7.24) when a shortcut defining GTEST_FOUND was introduced as soon as all gtest and gmock targets are defined:

find_package(GMock QUIET)
# the GMockConfig.cmake from the Debian package cmake-extras provides all targets
if(TARGET gtest AND TARGET gtest_main AND TARGET gmock AND TARGET gmock_main)
set(GMOCK_FOUND TRUE)
set(GTEST_FOUND TRUE)
else()

However, those targets might have been defined via a previous source build of gtest and gmock.
In this case, when entering the second catkin sub-package, all GTEST_* variables are undefined again, but the targets still exist causing GTEST_FOUND to be defined w/o defining the remaining variables.

This PR fixes this by defining both GTEST_* and GMOCK_* variables in a cached fashion in this case.

cmake/test/gtest.cmake Outdated Show resolved Hide resolved
@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 6, 2020

I see. Please suggest an alternative solution then to fix the broken state.

@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 6, 2020

Maybe, alternatively, we could repeat the code setting those variables where GMOCK_FOUND and GTEST_FOUND are originally defined in the offending situation:

# the GMockConfig.cmake from the Debian package cmake-extras provides all targets
if(TARGET gtest AND TARGET gtest_main AND TARGET gmock AND TARGET gmock_main)
set(GMOCK_FOUND TRUE)
set(GTEST_FOUND TRUE)
else()

To be on the really safe side, you might want to test the variables [GTEST | GMOCK]_FROM_SOURCE_* beforehand for existence.

@dirk-thomas
Copy link
Member

Please suggest an alternative solution then to fix the broken state.

Unfortunately I don't have a proposal at hand. This CMake logic is extremely fragile and I wouldn't consider a "cmake project that comprises multiple catkin sub-projects" to be a supported use case - at least this is not used / tested anywhere. So I am not surprised recent changes broke that use case. I am happy to review / consider any kind of patch if it looks reasonably safe.

we could repeat the code setting those variables where GMOCK_FOUND and GTEST_FOUND are originally defined in the offending situation:

I would wonder if this would only cover some of the cases (depending on where/how Googletest is found) but not correctly define the variable in other cases when including multiple catkin packages within a single CMake context. Please consider updating the pull request as you imagine it. Even if it doesn't solve all cases but improves the one you are mostly interested in that would be fine with me to merge.

@rhaschke rhaschke force-pushed the fix-gtest-lookup branch from 57d0daa to 350e429 Compare July 6, 2020 23:24
@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 6, 2020

@dirk-thomas, I pushed the suggested alternative solution.

@dirk-thomas
Copy link
Member

The patch looks fine to me. If it works for your use case 👍 from me.

Please target the default branch though which is noetic-devel. I am happy to backport the commit from there to kinetic-devel.

@dirk-thomas dirk-thomas added the bug label Jul 6, 2020
@dirk-thomas
Copy link
Member

Maybe also update the title to reflect the changed patch.

@rhaschke rhaschke force-pushed the fix-gtest-lookup branch from 350e429 to 9ff9b36 Compare July 6, 2020 23:54
@rhaschke rhaschke changed the base branch from kinetic-devel to noetic-devel July 6, 2020 23:54
@rhaschke rhaschke changed the title Cached definition of GTEST_* and GMOCK_* variables Define GMOCK_* and GTEST_* variables in a new subproject Jul 6, 2020
@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 6, 2020

Rebased onto noetic-devel and rephrased PR title. Thanks for back-porting to Melodic!

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 3628875 into ros:noetic-devel Jul 7, 2020
@dirk-thomas
Copy link
Member

Cherry-picked to kinetic-devel since CI had passed before already: 3e1c655.

@rhaschke rhaschke deleted the fix-gtest-lookup branch September 5, 2020 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants