[WIP] Modernization of CMake

[Nicogene]

This PR has the goal of introduce YCM has dependency (already a dependency of icub-main) and introduce the usage of InstallBasicPackageFiles.cmake that allows to automatically all the .cmake to make consume this library externally.

I treated icub-firmware-shared as header only following this example.

I am facing some problems:

• [ ] preserving the directory hierarchy (without using install(DIRECTORY).
• [ ] make consume it correctly by icub-main.

For the latter point the situation is more tricky. The devices reling on icub-firmware-shared inside icub-main both include and add to the sources the .h and .c of icub-firmware-shared.

I opened the PR to ask help, maybe I am pursuing the wrong path (cc @traversaro @drdanz)

It is super WIP, DO NOT MERGE.

[traversaro]

I just noticed that we always used 4.0.7 at the CMake version level, while we tagged this repos a 1.15.0, from the next release we may want to fix this.

[traversaro]

preserving the directory hierarchy (without using install(DIRECTORY).

If you don't want to use install(DIRECTORY), the only other logic that I think it make sense to use is the one described in https://stackoverflow.com/a/25933984 .

The devices reling on icub-firmware-shared inside icub-main both include and add to the sources the .h and .c of icub-firmware-shared.

Do you have a link for such usage in icub-main ?

[Nicogene]

In icub-main, for example ethResources has the following CMakeLists.txt:

https://github.com/robotology/icub-main/blob/master/src/libraries/icubmod/embObjLib/CMakeLists.txt

All the cmake variables (e.g. CORE_FOLDER EO_BASE_DIR etc) are set by embObjLib.cmake: https://github.com/robotology/icub-main/blob/master/src/libraries/icubmod/embObjLib/embObjLib.cmake

This cmake helper relies on the variable icub_firmware_shared_embobj_INCLUDE_DIR that it is not set by the automatically generated Config.cmake(the INCLUDE_DIR variable are not deprecated?).

Probably the ethResources CMakeLists.txt has to be changed accordingly, but adding:

target_link_libraries(${PROJECT_NAME} icub_firmware_shared::embobj)

Is not enought, because I should have the includes, but how can I add them in the add_library(${PROJECT_NAME}) phase?

I hope to have been clear, treat cmake stuff is always tricky for me :sweat:

[Nicogene]

If you don't want to use install(DIRECTORY), the only other logic that I think it make sense to use is the one described in https://stackoverflow.com/a/25933984 .

This point actually is not blocking, I can restore the install directory.

[drdanz]

Not a review yet, but please note that if I remember correctly, being a header only library, icub-firmware-shared, at the moment works from build, install and source dir, not just from build and install like most projects

[traversaro]

Probably the ethResources CMakeLists.txt has to be changed accordingly, but adding:

target_link_libraries(${PROJECT_NAME} icub_firmware_shared::embobj)

Is not enought, because I should have the includes, but how can I add them in the add_library(${PROJECT_NAME}) phase?

This should be done by the target_include_directories command at https://github.com/robotology/icub-firmware-shared/pull/31/files#diff-6c134fcd5ed07e356813d7192582fb9aR210 . But obviously this works correctly only if the passed include directories are correct. What are the "correct" includes does not depend only on the library that you export, but how the downstream code uses it. In this particular case it seems that the files from this library are included as #include "EoCommon.h" (see https://github.com/robotology/icub-main/blob/4f5881acafab693f763dd277f87a8996fdea01a3/src/tools/ethLoader/ethLoaderLib/BoardInfo.h#L17), so to be consistent the directory passed to target_include_directories for the installation case, i..e"$<INSTALL_INTERFACE:$/${CMAKE_INSTALL_INCLUDEDIR}>"` should point to exactly where the files are contained, that according to the code:

  install(FILES ${${LIBRARY_TARGET_NAME}_HDR}
          DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${LIBRARY_TARGET_NAME}")

is instead ${CMAKE_INSTALL_INCLUDEDIR}/${LIBRARY_TARGET_NAME} . An alternative is to change the downstream code to include the files as #include <embobj/EoCommon.h>, and in that case we should not change anything of the downstream code (but given how the files are contained in this repo, this would probably break the use from the source/build, as the the files are not contained in a directory called "embobj".

Furthermore, note that also the argument "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/embobj>" passed to target_include_directories is wrong as well.

[Nicogene]

An alternative is to change the downstream code to include the files as #include <embobj/EoCommon.h>, and in that case we should not change anything of the downstream code (but given how the files are contained in this repo, this would probably break the use from the source/build.

My purpose was to add this feature without changes the downstream code if possible, then I would discard this option. Probably I will have to change the downstream CMakeLists.txt but i would try to not touch the source code.

[traversaro]

Probably I will have to change the downstream CMakeLists.txt but i would try to not touch the source code.

Ok, then you need to modify accordingly the lines:

  target_include_directories(${LIBRARY_TARGET_NAME} INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/embobj>"
                                                              "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>")

to contains exactly the directory that contains the .h files, both in the case of the source tree (BUILD_INTERFACE) and the install prefix (INSTALL_INTERFACE).

[Nicogene]

Ok, then you need to modify accordingly the lines: target_include_directories(${LIBRARY_TARGET_NAME} INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/embobj>" "$<INSTALL_INTERFACE:$/${CMAKE_INSTALL_INCLUDEDIR}>")

@traversaro this (https://github.com/robotology/icub-firmware-shared/pull/31/commits/1b54492dd40195fcb991422f700aad297c177221) is what you mean?

[traversaro]

@traversaro this (1b54492) is what you mean?

Yes, even if having such an articulated directory structure is not common if then the headers are meant to be included as #include "headerFileName.h", but if this is how the library is meant to be used, this is correct.

[Nicogene]

but if this is how the library is meant to be used, this is correct.

Speaking of, it is not clear to me the structure of this library. I saw that every module/library inside icub-main that uses embobj part, use the cmake helper embobjLib.cmake to populate some cmake variables defining the sub-parts of embobj.

The strange thing for me is that this helper is inside icub-main and not in this repository, and it is not exported/installed, then if another project need to use icub-firmware-shared can't use it.

This problem arose in the diagnostic development, since it is external to icub-main.

cc @marcoaccame @traversaro @pattacini @triccyx

[pattacini]

Uhm, it shall be located in icub-firmware-shared then @Nicogene.

Anyway, bear in mind that icub-main shall compile also without icub-firmware-shared thus things requiring that specific CMake module need to be put under condition guard.

[traversaro]

I think that the only practical effect of the embobjLib.cmake is to include the directories https://github.com/robotology/icub-main/blob/master/src/libraries/icubmod/embObjLib/embObjLib.cmake#L87, so I think that as long as we migrate the usage of the library to use the imported target, probably its use it can be dropped.

[Nicogene]

The development became more complex than expected and it has been move to this branch: https://github.com/robotology/icub-firmware-shared/tree/feat/cmakePowerUp

Closing this PR, it has been useful for presenting a draft and discussing, we will open a new one as soon as it will be ready.