roboticslab-uc3m / questions-and-answers

A place for general debate and question&answer
https://robots.uc3m.es/developer-manual/appendix/repository-index.html
2 stars 0 forks source link

Fully embrace a target-driven approach in CMake exported config #44

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

In some legacy code, we were used to see <Project>Config.cmake(.in) files with contents that resembled the following lines (source):

SET(KINEMATICS_DYNAMICS_MODULE_PATH "@KINEMATICS_DYNAMICS_MODULE_PATH@")
SET(KINEMATICS_DYNAMICS_DEFINES "@KINEMATICS_DYNAMICS_DEFINES@")
SET(KINEMATICS_DYNAMICS_INCLUDE_DIRS "@KINEMATICS_DYNAMICS_INCLUDE_DIRS@")
SET(KINEMATICS_DYNAMICS_LINK_DIRS "@KINEMATICS_DYNAMICS_LINK_DIRS@")
SET(KINEMATICS_DYNAMICS_LIBRARIES "@KINEMATICS_DYNAMICS_LIBRARIES@")

These are now relics from the pre-target era, and as such, the <project>_INCLUDE_DIRS, <project>_LINK_DIRS and <project>_DEFINES variables (holding what is better known as usage requirements) no longer make sense when a <project>Targets.cmake file is finally being generated and exported by CMake along with each successful configuration. The <project>_LIBRARIES var is a mere alias for all exported targets, created plainly for convenience, but might (and perhaps should) be dropped in order to favour explicitness in downstream calls to target_link_libraries().

This is how said <package>Config.cmake.in template file looks now (source):

set(ROBOTICSLAB_KINEMATICS_DYNAMICS_INCLUDE_DIRS)

foreach(_dir @PACKAGE_ROBOTICSLAB_KINEMATICS_DYNAMICS_INCLUDE_DIR@)
    set_and_check(_temp_var ${_dir})
    list(APPEND ROBOTICSLAB_KINEMATICS_DYNAMICS_INCLUDE_DIRS ${_temp_var})
endforeach()

if(NOT "@_exported_targets@" STREQUAL "")
    include(${CMAKE_CURRENT_LIST_DIR}/ROBOTICSLAB_KINEMATICS_DYNAMICSTargets.cmake)

    set(ROBOTICSLAB_KINEMATICS_DYNAMICS_LIBRARIES)

    foreach(_target @_exported_targets@)
        list(APPEND ROBOTICSLAB_KINEMATICS_DYNAMICS_LIBRARIES ROBOTICSLAB::${_target})
    endforeach()
endif()

The main focus lies now on the following line:

include(${CMAKE_CURRENT_LIST_DIR}/ROBOTICSLAB_KINEMATICS_DYNAMICSTargets.cmake)

A configuration file might be as simple as that, see CMake docs. Apart from said line, there is some safety boilerplate, as well as the aforementioned <package>_LIBRARIES alias and more checks and sets for <package>_INCLUDE_DIRS.

The reason behind keeping a variable to point at where headers are stored is linked to the essence of YARP interfaces. In fact, files such as ICartesianControl.h, ICartesianSolver.h, etc. (ref) do not belong to a specific library and therefore didn't undergo the common target mechanism standardization. However, CMake supports the so-called interface targets, suited for header-only libraries that do not generate any output binaries.

In this issue, I propose introducing interface targets for YARP stand-alone interface headers and dropping any exported variable that can be fully replaced by the target mechanism.

Also, the <package>_MODULE_PATH variable setter could be rewritten as one or more calls to the include() command, one per module directory. It is to be considered whether such behavior (automatic inclusion upon returning from find_package()) is desired.

PeterBowman commented 6 years ago

Hint: YARP::YARP_conf is an INTERFACE target, it links to generated configuration-related headers.

PeterBowman commented 6 years ago

Note: interface targets may set their own dependencies, too. Example: ICartesianControl.h depends on yarp::os::Vocab.

PeterBowman commented 6 years ago

Example: asrob-uc3m/libraries/YarpPlugins/CMakeLists.txt.

PeterBowman commented 6 years ago

Disclaimer: INTERFACE targets did not exist prior to CMake 3.0.

PeterBowman commented 6 years ago

See https://github.com/roboticslab-uc3m/developer-manual/issues/18#issuecomment-365926298 regarding YCM integration.

PeterBowman commented 6 years ago

I've touched several repos, but the most relevant changes happened here:

The usual <PREFIX>_INCLUDE_DIRS and <PREFIX>_LIBRARIES CMake export variables have been dropped. They turned into:

In both cases, usage requirements will propagate uniformly through target_link_libraries, that is, there is no need to call (target_)include_directories (except in those cases that do not propagate headers through library nor interface targets).

Also, the _MODULE_PATH variable setter could be rewritten as one or more calls to the include() command, one per module directory. It is to be considered whether such behavior (automatic inclusion upon returning from find_package()) is desired.

We barely use utility modules (e.g. InstallOpenravePlugin.cmake), most are find-modules that should not be include'd like YARP does with YarpPlugin.cmake and other utilities (ref).