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

Add section for YCM/cmake common practices #18

Open
PeterBowman opened this issue Dec 22, 2017 · 20 comments
Open

Add section for YCM/cmake common practices #18

PeterBowman opened this issue Dec 22, 2017 · 20 comments

Comments

@PeterBowman
Copy link
Member

PeterBowman commented Dec 22, 2017

Gather a few examples of usual pitfalls, recommended usage and hints like roboticslab-uc3m/questions-and-answers#39 (comment).

Also: roboticslab-uc3m/questions-and-answers#18

@PeterBowman
Copy link
Member Author

PeterBowman commented Jan 17, 2018

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 jgvictores changed the title Add section for YCM common practices Add section for YCM/cmake common practices Feb 10, 2018
@jgvictores
Copy link
Member

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

Added link to roboticslab-uc3m/questions-and-answers#18 in description.

@PeterBowman
Copy link
Member Author

...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
Copy link
Member

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
Copy link
Member Author

PeterBowman commented Feb 16, 2018

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
Copy link
Member Author

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

@jgvictores
Copy link
Member

roboticslab-uc3m/kinematics-dynamics@6063082 works! Thank you so much!!

@PeterBowman
Copy link
Member Author

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

@PeterBowman
Copy link
Member Author

PeterBowman commented Mar 23, 2018

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 roboticslab-uc3m/questions-and-answers#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 added a commit to roboticslab-uc3m/amor-main that referenced this issue Mar 23, 2018
PeterBowman added a commit to roboticslab-uc3m/asibot-main that referenced this issue Mar 23, 2018
@PeterBowman
Copy link
Member Author

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 roboticslab-uc3m/yarp-devices@2fdc802.

@PeterBowman
Copy link
Member Author

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 roboticslab-uc3m/questions-and-answers#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
Copy link
Member Author

We haven't spoken about CPack, yet, but here is another guideline for superbuild repos. Per asrob-uc3m/robotDevastation@cfac906, this:

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

is better than this:

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

@PeterBowman
Copy link
Member Author

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 added a commit to roboticslab-uc3m/yarp-devices that referenced this issue Apr 4, 2018
@PeterBowman
Copy link
Member Author

PeterBowman commented Apr 4, 2018

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:

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 '[email protected]'`; 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 robotology/ycm-cmake-modules#225.

PeterBowman added a commit to roboticslab-uc3m/kinematics-dynamics that referenced this issue Apr 4, 2018
@PeterBowman
Copy link
Member Author

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.

@PeterBowman
Copy link
Member Author

PeterBowman commented Apr 6, 2018

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 #18 (comment), 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 roboticslab-uc3m/questions-and-answers#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: roboticslab-uc3m/yarp-devices#183 arised because of non-centralized CMake options (and my lack of care, of course!).

@PeterBowman
Copy link
Member Author

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
Copy link
Member Author

Most of the YCM hacks laid out in previous comments (and mainly #18 (comment)) are now outdated due to the outcome of roboticslab-uc3m/questions-and-answers#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
Copy link
Member Author

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

@PeterBowman
Copy link
Member Author

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 roboticslab-uc3m/questions-and-answers#78. We don't care about that anymore on non-superbuild projects s since YCM is regarded as a hard dependency by them (roboticslab-uc3m/questions-and-answers#66).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants