Add section for YCM/cmake common practices

PeterBowman commented 6 years ago

Gather a few examples of usual pitfalls, recommended usage and hints like https://github.com/roboticslab-uc3m/questions-and-answers/issues/39#issuecomment-353584524.

Also: https://github.com/roboticslab-uc3m/questions-and-answers/issues/18

PeterBowman commented 6 years ago

A few remarks on YCM based on my experience:

If working on a superbuild-type repo, everything is fine.
If not working on a superbuild-type repo, YCM:
- ...copes well with header-only repos (e.g. ColorDebug.hpp).
- ...requires ugly hardcoding when dealing with compiled repos (i.e. libraries).
- ...is unable to load most CMake files on configure time (e.g. ColorDebugOptions.cmake), at least on the first attempt.

For the record, most of this was discovered at roboticslab-uc3m/color-debug#6.

jgvictores commented 6 years ago

Updated issue title to "Add section for YCM/cmake common practice" (added cmake).

Added link to https://github.com/roboticslab-uc3m/questions-and-answers/issues/18 in description.

PeterBowman commented 6 years ago

...copes well with header-only repos (e.g. ColorDebug.hpp).

I'll give some pointers on this, based on two real use cases.

Header-only repos + include paths propagated via (target_)include_directories. Example: roboticslab-uc3m/color-debug (see README).

Config file (COLOR_DEBUGConfig.cmake):
- no targets exported
- sets COLOR_DEBUG_INCLUDE_DIRS for use in (target_)include_directories by downstreams

YCM build file (e.g. BuildCOLOR_DEBUG.cmake at kinematics-dynamics):

include(YCMEPHelper)

ycm_ep_helper(COLOR_DEBUG TYPE GIT
            STYLE GITHUB
            REPOSITORY roboticslab-uc3m/color-debug.git
            TAG master)

# Include path to ColorDebug.hpp.
ExternalProject_Get_Property(COLOR_DEBUG INSTALL_DIR)
include_directories(${INSTALL_DIR}/${CMAKE_INSTALL_INCLUDEDIR})

a few lines regarding how to handle CMake modules were omitted in the previous snippet
note the global include_directories call, that is, every subdir processed after find_or_build_package(COLOR_DEBUG) will learn how to find color-debug headers

Root CMakeLists.txt downstream (e.g. kinematics-dynamics):

include(FindOrBuildPackage)
find_or_build_package(COLOR_DEBUG)

# find_or_build_package() doesn't bring into scope COLOR_DEBUG_INCLUDE_DIRS
# nor other config variables. If a system copy is found, we have to call the
# find_package() command and proceed as usual.
if(USE_SYSTEM_COLOR_DEBUG)
  find_package(COLOR_DEBUG REQUIRED)
  include_directories(${COLOR_DEBUG_INCLUDE_DIRS})
  add_library(COLOR_DEBUG UNKNOWN IMPORTED)
endif()

in case a COLOR_DEBUGConfig.cmake file is found on system (either the package was installed or simply cloned and configured locally), we call include_directories here AND create a dummy UNKNOWN target so that add_dependencies does not crash (see next point)

Component-based CMakeLists.txt (e.g. KdlVectorConverterLib at kinematics-dynamics):
```
add_library(KdlVectorConverterLib ...)
add_dependencies(KdlVectorConverterLib COLOR_DEBUG)
```
- in components that use color-debug headers, in order to allow multi-core compilation when color-debug itself is pulled by YCM, build order must be preserved with add_dependencies

Related issues: roboticslab-uc3m/color-debug#6, roboticslab-uc3m/color-debug#8.

Header-only repos + include paths propagated via target_link_libraries and usage requirements. Example: asrob-uc3m/yarp-devices + asrob-uc3m/robotDevastation.

Creation and export of INTERFACE targets (e.g. YarpPlugins/CMakeLists.txt + YARP device interface headers):
```
# Device interface target.
add_library(RobotInterfaces INTERFACE)

target_include_directories(RobotInterfaces INTERFACE $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
                                                   $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)

# Install interface headers.
install(FILES IRobotManager.hpp
      DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

# Register export set.
install(TARGETS RobotInterfaces
      EXPORT ASROB_YARP_DEVICES)
```
- usage requirements, i.e. include paths, are conveniently set via target_include_directories
- header files must be installed in an individual manner, that is, with install(FILES)
- we don't really need a <pkg>Config.cmake file here unless more variables need to be exported
- since interface headers are grouped within a target, CMake takes care of this and configures the exported settings in <pkg>Targets.cmake (via CMakePackageConfigHelpers)

YCM build file (e.g. BuildASROB_YARP_DEVICES.cmake):

include(YCMEPHelper)

ycm_ep_helper(ASROB_YARP_DEVICES TYPE GIT
            STYLE GITHUB
            REPOSITORY asrob-uc3m/yarp-devices.git
            TAG master)

# Include path to device interface headers.
ExternalProject_Get_Property(ASROB_YARP_DEVICES INSTALL_DIR)
include_directories(${INSTALL_DIR}/${CMAKE_INSTALL_INCLUDEDIR})

# Needed by dependencies on configure time.
if(NOT TARGET ASROB::RobotInterfaces)
  # UNKNOWN breaks on executable targets, use INTERFACE
  add_library(ASROB::RobotInterfaces INTERFACE IMPORTED)
endif()

unfortunately, there still has to be a hardcoded include_directories call somewhere; in this case, only for repos pulled by YCM
targets imported by YCM are not visible on configure time, hence we need to create dummy INTERFACE targets (take heed of the comment)

Root CMakeLists.txt downstream (e.g. asrob-uc3m/robotDevastation):
```
include(FindOrBuildPackage)
find_or_build_package(ASROB_YARP_DEVICES)

if(USE_SYSTEM_ASROB_YARP_DEVICES)
  find_package(ASROB_YARP_DEVICES REQUIRED)
  add_library(ASROB_YARP_DEVICES UNKNOWN IMPORTED)
endif()
```
- similar to the color-debug use case, but we can spare a global include_directories call
- note that the dummy target here refers to the package name (the same one handled by ycm_ep_helper in BuildASROB_YARP_DEVICES.cmake), not to the imported interface target(s)
Component-based CMakeLists.txt (e.g. UtilsLib at asrob-uc3m/robotDevastation)
```
add_library(UtilsLib Hub.hpp ...)
add_dependencies(UtilsLib ASROB_YARP_DEVICES)
target_link_libraries(UtilsLib PUBLIC ASROB::RobotInterfaces ...)
```
- this is the nice way since the only components (targets) that need to be aware of those interface headers are the ones that actually need them; of course, this is only true for a system-installed yarp-devices repo, remember that YCM forces a global include_directories call prior to that
- targets that depend on UtilsLib but not on ASROB::RobotInterfaces will know how to find the interface headers, too, thanks to the propagation of usage requirements via target_link_libraries

Related issues: roboticslab-uc3m/questions-and-answers#44, asrob-uc3m/robotDevastation#122.

jgvictores commented 6 years ago

Regarding this setup, I've observed the following behavior on two different machines: no gtests -> no tests -> sudo apt install libgtest-dev -> only way to enable tests is by completely erasing the cmake cache. The behavior I'm used to is being able to enable a component in an online fashion, installing packages and enabling components within the same ccmake session.

PeterBowman commented 6 years ago

Regarding this setup, I've observed the following behavior on two different machines: no gtests -> no tests -> sudo apt install libgtest-dev -> only way to enable tests is by completely erasing the cmake cache.

I noticed that both GTestSources_SOURCE_DIR and GTestSources_INCLUDE_DIR are correctly set (ref), but GTestSources_FOUND/GTESTSOURCES_FOUND keeps evaluating to FALSE. Adding a FOUND_VAR parameter to find_package_handle_standard_args did not help (see FindPackageHandleStandardArgs, downstream CMake list file). Disclaimer: ROS messes up the removal of libgtest-dev package, i.e. you cannot uninstall googletest without removing ROS (+ dependencies).

PeterBowman commented 6 years ago

@jgvictores fixed at https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/6063082a298605d9202cc2ed17793b0929a2c133, I'll update the FindGTestSources.cmake modules across roboticslab-uc3m and asrob-uc3m.

jgvictores commented 6 years ago

https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/6063082a298605d9202cc2ed17793b0929a2c133 works! Thank you so much!!

PeterBowman commented 6 years ago

Regarding CMake common practices, I'd draw a few conclusions from https://github.com/roboticslab-uc3m/questions-and-answers/issues/44 and sum them up in the manual.

PeterBowman commented 6 years ago

Thanks to https://travis-ci.org/roboticslab-uc3m/amor-main/jobs/356811908 I found out that superbuild-like repos should never enforce a lower YCM version (via YCM_TAG, example) than any of its orchestrated projects. That is, if amor-main downloads kinematics-dynamics, which requires YCM 0.6.0 or later, don't set(YCM_TAG v0.2.2) in root CMakeLists.txt at amor-main.

Remember that pulling specific releases of YCM originated at https://github.com/roboticslab-uc3m/questions-and-answers/issues/24. Anyway, I'd recommend pulling YCM master for superbuild-like repos. If any upstream problem arises, we'll learn of that thanks to daily CI cron jobs. We only have to make sure that current YCM is compatible with the version number set in cmake_minimum_required.

PeterBowman commented 6 years ago

Another CMake guideline proposal: set find_package(<pkg> REQUIRED) in root CMakeLists.txt for hard dependencies. Only a few repos in our org will meet this criteria: YARP in yarp-devices, OpenRAVE+Boost in openrave-yarp-plugins. See https://github.com/roboticslab-uc3m/yarp-devices/commit/2fdc802cb7eecd061af11829a3ade5e93c56b224.

PeterBowman commented 6 years ago

Only a few repos in our org will meet this criteria

In spite of that, we can assume that most repos just need YARP for whatever purpose they exist for. When working on https://github.com/roboticslab-uc3m/questions-and-answers/issues/45, we observed that certain components inside share/ and libraries/YarpPlugins/ may require having set common installation variables (via yarp_configure_external_installation()). Thus, pulling YARP-related commands to root CMakeLists.txt will avoid code duplication and other potential issues.

PeterBowman commented 6 years ago

We haven't spoken about CPack, yet, but here is another guideline for superbuild repos. Per https://github.com/asrob-uc3m/robotDevastation/commit/cfac906c217ad81f9cd2aff572ef71282893d31c, this:

 install(DIRECTORY ${CMAKE_BINARY_DIR}/install/
         DESTINATION "./")

is better than this:

 install(DIRECTORY ${CMAKE_BINARY_DIR}/install/
         DESTINATION ${CMAKE_INSTALL_PREFIX})

PeterBowman commented 6 years ago

Regarding CMake options, care must be taken to ensure that:

options that depend on other options are processed in the right order;
multiple checks of the same option always follow the option declaration (via option() or cmake_dependent_option() or yarp_prepare_plugin()).

That is, path traversal matters. It's advisable to respect the following sequence at root CMakeLists.txt:

add_subdirectory(cmake) # regular, find- and ycm-modules
add_subdirectory(libraries) # libraries to link against
add_subdirectory(programs) # apps that may depend on one or more libraries
add_subdirectory(tests) # should come last, but...
add_subdirectory(share) # ...we may define dirs here that depend on ENABLE_tests, too

In relation to this last line, see roboticslab-uc3m/yarp-devices/share/CMakeLists.txt.

PeterBowman commented 6 years ago

Another one: in Travis, set -DNON_INTERACTIVE_BUILD:BOOL=ON (or, at least, check what happens). Per YCM docs:

For build machines the NON_INTERACTIVE_BUILD variable should be set to true.

Note: This should either be set by running cmake -DNON_INTERACTIVE_BUILD:BOOL=TRUE, or using an initial cache file and running cmake -C <file>

Tracing this variable back to YCM sources:

robotology/ycm/modules/YCMEPHelper.cmake#L196-L199:

if(NOT NON_INTERACTIVE_BUILD)
 # Non interactive builds should always perform the update step
 set_property(DIRECTORY PROPERTY EP_SCM_DISCONNECTED 1)
endif()

robotology/ycm/modules/YCMEPHelper.cmake#L259-L261:

if(NOT NON_INTERACTIVE_BUILD AND _error_code)
 message(FATAL_ERROR "Failed to get git name. Please set it with \"git config --global user.name Firstname Lastname\" or from your favorite git gui.")
endif()

In fact, we ended up copying around the following lines in several .travis.yml files (ref):

#-- Configure Git (needed by YCM)
- if [ ! `git config --get user.email` ]; then `git config --global user.email 'user@example.com'`; fi
- if [ ! `git config --get user.name` ]; then `git config --global user.name 'Travis CI'`; fi

Edit: (from robotology/ycm/cmake-next/proposed/ExternalProject.cmake, that is, set YCM_USE_CMAKE_PROPOSED to ON)

# If ``SCM_DISCONNECTED`` is set, the update step is not executed
# automatically when building the main target. The update step can still
# be added as a step target and called manually. This is useful if you
# want to allow to build the project when you are disconnected from the
# network (you might still need the network for the download step).
# This is disabled by default.
# The directory property ``EP_SCM_DISCONNECTED`` can be used to change
# the default value for all the external projects in the current
# directory and its subdirectories.

Edit2: see https://github.com/robotology/ycm/issues/225.

PeterBowman commented 6 years ago

I reverted https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/ebfe41e7bb016d441b61223050f48b4c021beaaf. Not sure why it worked without -DYCM_USE_CMAKE_PROPOSED=ON (I forgot to add that in the original commit) and, then, without -DNON_INTERACTIVE_BUILD=ON at the amor-api step. CI builds: original commit, no NON_INTERACTIVE_BUILD. Also, running this on docker and asibot-main caused trouble at the first orchestrated repo that required YCM (e.g. kinematics-dynamics), because of missing git configuration.

PeterBowman commented 6 years ago

As spoken with @jgvictores, we strive to keep all declarations as close as possible to the ~~place~~ scope they are ~~first~~ used at. This is extendable to CMake options and the usual option()/cmake_dependent_option()/yarp_prepare_plugin() calls (example). However, as noted in https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-378567758, there are cases we'd better arrange all options at once and make them available to all subfolders. As suggested, roboticslab-uc3m/amor-api/cmake/AMOR_APIOptions.cmake (private repo) is the currently proposed location and module name.

Also, there are cases in which CMake variables defined by Config.cmake or Find.cmake must be accessible in distinct folders. In order to avoid redundant find_package() calls, those could be arranged in one place as well (@jgvictores proposes cmake/<pkg>Dependencies.cmake). This resembles https://github.com/roboticslab-uc3m/questions-and-answers/issues/54, but covers soft dependencies, too.

Edit: scope vs order in which options are declared/defined. The former improves readability (one may expect that options defined in subfolders are not visible to sibling folders), but the latter is what actually happens (variables are stored in a global cache).

Edit 2: https://github.com/roboticslab-uc3m/yarp-devices/issues/183 arised because of non-centralized CMake options (and my lack of care, of course!).

PeterBowman commented 6 years ago

In relation to this last line, see roboticslab-uc3m/yarp-devices/share/CMakeLists.txt.

Old style:

# root/components/
if(ENABLE_mycomponent)
    add_subdirectory(mycomponent)
endif()

New preferred style:

# root/components/
add_subdirectory(mycomponent)

and

# root/components/mycomponent/
if(ENABLE_mycomponent)
    # do something
endif()

PeterBowman commented 6 years ago

Most of the YCM hacks laid out in previous comments (and mainly https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-365926298) are now outdated due to the outcome of https://github.com/roboticslab-uc3m/questions-and-answers/issues/55: in non-superbuild repos, YCM should be consumed as a hard dependency, thus treating all dynamically pulled repos as hard deps, too (e.g. color-debug).

PeterBowman commented 6 years ago

Just found CGold: The Hitchhiker’s Guide to the CMake, still WIP. @jgvictores you might consider adding this to asrob-uc3m/tutoriales.

PeterBowman commented 5 years ago

I reverted roboticslab-uc3m/kinematics-dynamics@ebfe41e. Not sure why it worked without -DYCM_USE_CMAKE_PROPOSED=ON (I forgot to add that in the original commit) and, then, without -DNON_INTERACTIVE_BUILD=ON at the amor-api step. CI builds: original commit, no NON_INTERACTIVE_BUILD. Also, running this on docker and asibot-main caused trouble at the first orchestrated repo that required YCM (e.g. kinematics-dynamics), because of missing git configuration.

I'm going to fiddle with this again on superbuild repos at https://github.com/roboticslab-uc3m/questions-and-answers/issues/78. We don't care about that anymore on non-superbuild projects s since YCM is regarded as a hard dependency by them (https://github.com/roboticslab-uc3m/questions-and-answers/issues/66).

roboticslab-uc3m / developer-manual

Add section for YCM/cmake common practices #18