search

Jmeyer1292 / robot_cal_tools

A suite of tools focused on calibration of sensors for robotic workcell development
Apache License 2.0
140 stars 40 forks source link

Fresh clone doesn't compile due to missing GTest #83

Open jdlangs opened 4 years ago

jdlangs commented 4 years ago

When you clone the repo down and attempt to build as a catkin workspace, rct_ros_tools fails to build due to the local copy/build of GTest not being present. I was able to figure out I could build with RCT_BUILD_TESTS to get it working but that shouldn't be needed when starting with the repo.

marip8 commented 4 years ago

Did you happen to be building with CATKIN_ENABLE_TESTING=ON? I made a change recently that makes rct_ros_tools use the version of GTest that is cloned into the workspace by rct_common (which happens when RCT_BUILD_TESTS=ON). It seems like there is a scenario in which CATKIN_ENABLE_TESTING=ON but RCT_BUILD_TESTS=OFF in which the build would probably fail like you described.

A solution to this case might be amending the test section in rct_ros_tools/CMakeLists.txt like this:

if(CATKIN_ENABLE_TESTING && RCT_BUILD_TESTS)
...
endif()
jdlangs commented 4 years ago

Yes, that's the failure mode. Those are the default values for the variables, right? At least I didn't explicitly turn on CATKIN_ENABLE_TESTING.

Your proposed solution is probably the simplest. I was thinking of something like printing a warning that testing will be disabled until it's built with RCT_BUILD_TESTS on.

marip8 commented 4 years ago

A quick search seems to indicate that the default value of CATKIN_ENABLE_TESTING is ON.

Alternatively we could fix it with:

if(CATKIN_ENABLE_TESTING)
  find_package(rostest REQUIRED)
  if(RCT_BUILD_TESTS)
    find_package(GTest REQUIRED)
  endif()
  ...
endif()

I think this would make it use the version of GTest provided by rostest (which is different than the version we clone) in the case that RCT_BUILD_TESTS=OFF and would allow building the unit tests in the more standard catkin way where only the CATKIN_ENABLE_TESTING variable is required

gavanderhoorn commented 4 years ago

IIRC, the default CMake variable for this would be BUILD_TESTING.

I seem to recall Catkin and Colcon both also set that variable, either depending on the value of CATKIN_ENABLE_TESTING in the case of Catkin, or some other conditional for Colcon.

Would perhaps renaming RCT_BUILD_TESTS to BUILD_TESTING solve this?

gachiemchiep commented 3 years ago

I fixed this problem in this pull : https://github.com/Jmeyer1292/robot_cal_tools/pull/95

Reference : ros gtest