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

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

Closed
PeterBowman opened this issue Jan 24, 2018 · 6 comments
Closed
Assignees
Labels

Comments

@PeterBowman
Copy link
Member

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

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

@PeterBowman
Copy link
Member Author

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

@PeterBowman
Copy link
Member Author

@PeterBowman
Copy link
Member Author

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

@PeterBowman
Copy link
Member Author

See roboticslab-uc3m/developer-manual#18 (comment) regarding YCM integration.

@PeterBowman
Copy link
Member Author

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:

  • <PREFIX>_INCLUDE_DIRS: new INTERFACE targets such as YarpDevicesInterfaces, KinematicsDynamicsInterfaces, etc. These hold the include paths to our YARP device interfaces.
  • <PREFIX>_LIBRARIES: individual, explicit calls to exported targets. Similarly, I chose to link against YARP::YARP_OS, YARP::YARP_dev, etc. instead of using ${YARP_LIBRARIES}.

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).

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

No branches or pull requests

1 participant