make rostest build dependency in CMakeLists.txt optional

[dirk-thomas]

(Originally filled by @bulwahn at ros/catkin#588)

We want to make the rostest build dependency in the CMakeLists.txt of all ROS packages optional.

This shall been done by the refactoring the CMakeLists.txt from:

find_package(catkin REQUIRED COMPONENTS [...] roscpp rostest)
[...]
if(CATKIN_ENABLE_TESTING)
  add_subdirectory(test)
endif()

to:

find_package(catkin REQUIRED COMPONENTS [...] roscpp)   [! drop rostest !]
[...]
if(CATKIN_ENABLE_TESTING)
  find_package(rostest)  [! add this line in the CATKIN_ENABLE_TESTING conditional block !]
  add_subdirectory(test)
endif()

For commits addressing this task, their commit message shall refer to this issue by
make rostest in CMakeLists optional (ros/rosdistro#3010).

This has been discussed on the ros-sig-buildsystem [1], and prepares for the roll-out of the REP 140 [2].

[1] https://groups.google.com/forum/#!topic/ros-sig-buildsystem/Ub7ktrLPSe0
[2] https://github.com/jack-oquin/rep/blob/master/rep-0140.rst

[dirk-thomas]

Just a comment stated earlier: this requires REP 140 to be active, implemented and rolled out. Otherwise you can not get rid of the build dependency in the package.xml file. The time frame for this is still not yet clear based on the slow progress in the last year.

[bulwahn]

To clarify the task in this issue, this is limited to the changes in the CMakeLists.txt file.
The package.xml will not be modified. Hence, this means that the CMakeLists.txt does not require the rostest package if CATKIN_ENABLE_TESTING is not set to 1, but the overall build system still will require it.

[bulwahn]

As rostest is still a build dependency for now, this change is only a first step that remains ineffective for most use cases, but mainly prepares for the roll-out of REP 140. However, the installation routines for OpenEmbedded [1] rely only on the CMakeLists, and not on the information in the package.xml. So, it can exploit this refined setup before REP 140 is in place, and explore the possible problematic cases for REP 140's roll-out.

[dirk-thomas]

We just applied the change to geometry and found and issue with the proposed approach. When find_package(rostest) is invoked that will reset the catkin variables (catkin_INLCUDE_DIRS, catkin_LIBRARIES, etc.).

While that does not affect Python testing it will lead to missing symbols when trying to link test executables without the collect libraries of found catkin packages.

We likely have to change the behavior of catkin to support this use case (only set the variables when being invoked with find_package(catkin COMPONENTS ...). But since that change significant it might not be backported to older distro. That would imply that any package using this new approach and using C++ tests should only apply the change on its Indigo branch.

[jack-oquin]

I've been wondering about this problem. In packages I maintain, it generally works OK because there is seldom anything left to build at the end of the CMakeLists.txt where tests are defined.

When there are test programs to compile under CATKIN_ENABLE_TESTING, what is the recommended approach? Should we do something like this?

if (CATKIN_ENABLE_TESTING)
  add_executable(${PROJECT_NAME}_trigger_node tests/trigger_node.cpp)
  target_link_libraries(${PROJECT_NAME}_trigger_node
                        ${catkin_LIBRARIES}
                        ${DC1394_LIBRARY})
  find_package(catkin REQUIRED COMPONENTS rostest)
  add_rostest(tests/no_device_node.test)
  add_rostest(tests/no_device_nodelet.test)
endif()

[wjwwood]

But since that change significant it might be backported to older distro.

@dirk-thomas did you mean "might not be backported"?

[dirk-thomas]

@jack-oquin Finding rostest through catkin is neither necessary nor recommended.

Currently the order to first build all test target (while the catkin variables are intact) and then find rostest before registering the tests is the only way which works. But since the order should not be completely different then outside of the testing block the referenced change in catkin aims to allow finding rostest early without affecting the catkin variables.

@wjwwood At the time of writing I thought the patch might not be backported. But after some further discussion it might be viable to backport it.

[jack-oquin]

Finding rostest through catkin is neither necessary nor recommended.

OK.

What is necessary and recommended?

[dirk-thomas]

It is necessary to find_package(rostest) since the package wants to use its CMake functions/macros.

It is not necessary to find_package(catkin) since the package does not want the catkin_* variables and especially does not want them to be overwritten.

The recommended way is what is posted in the first comment of this ticket. If the package has C++ unit tests which requires additional packages as dependencies the user may either:

find these dependencies individually and use their variables (just showing the libraries in the example):

find_package(dep1)
find_package(dep2)
...
target_link_libraries(my_test_target ${catkin_LIBRARIES} ${dep1_LIBRARIES} ${dep2_LIBRARIES})

or use catkin but take care of saving the variable content before:

set(saved_LIBRARIES ${catkin_LIBRARIES})
find_package(catkin REQUIRED COMPONENTS dep1 dep2)
...
target_link_libraries(my_test_target ${saved_LIBRARIES} ${catkin_LIBRARIES})

[jack-oquin]

So, the recommended version of my example would be:

if (CATKIN_ENABLE_TESTING)
  add_executable(${PROJECT_NAME}_trigger_node tests/trigger_node.cpp)
  target_link_libraries(${PROJECT_NAME}_trigger_node
                        ${catkin_LIBRARIES}
                        ${DC1394_LIBRARY})
  find_package(rostest REQUIRED)
  add_rostest(tests/no_device_node.test)
  add_rostest(tests/no_device_nodelet.test)
endif()

The only change being `find_package()``?

[dirk-thomas]

LGTM.

For the more difficult case where your test executable needs to compile/link against additional packages which need to be find_package()-ed in the test block we will need a different more complex template. I am not sure which of my previous options is less weird / more intuitive though.

[dirk-thomas]

I will close this since we there are no action items left based on the discussion. It is possible to workaround it within the CMake file of a package as illustrated by the example. Please feel free to comment with more information if you think this still needs some work.