CMake > 3.11 and YCM dependency

[diegoferigo]

The current README, for what concerns the YCM dependency, recommends either vendoring the three files we require, or installing YCM externally.

Starting from CMake 3.11, as many of you already know well, a new FetchContent module was introduced. It allows downloading and configuring external projects during the configuration phase of the project (contrarily to ExternalProject). Therefore, it is an excellent solution for dependencies like YCM.

We should add in the README also this option. Sadly, the default CMake version on the current Ubuntu 18.04 LTS is still 3.10 though.

For a minimal example you can refer to the following implementation:

• Main CMakeLists.txt
• BootstrapYCM.cmake

cc @traversaro @claudiofantacci @pattacini @drdanz

[traversaro]

I am still a bit conflicted regarding how we should recommend to handle optional dependencies such as the YCM in this case. In my experience, taking automatic decision based on whether a package is available on the system or not is typically a source of problems.
For example, in your example, what happens if a YCM older then the version required by gym-ignition is found in the system? What happens now is that the system YCM is considered suitable to be used, but it is actually not, so probably some obscure error will be given to the user. If instead you ask for a minimum version of YCM in your CMakeLists, and the system YCM is not enough, that is even worse: gym-ignition will install its own YCM, overwriting the system YCM if it is instlalled in the same prefix were gym-ignition is installed.
To mitigate this problem, I typically (see for example https://github.com/robotology/gazebo-fmi/blob/master/CMakeLists.txt#L66) I just use the inspection in the system just to initialize a variable USE_SYSTEM_<dep>, and then I call find_package or FetchContent based on the value of this variable. The main advantage is that USE_SYSTEM_<dep> can be manually set by any superbuild/build tool/catkin/ament/docker/snap/flatpak/vcpkg/homebrew/distro packaging scripts, that will result in a clear error if the system dep is not found when instead it is expected to be found. For example, in the superbuild I would always set USE_SYSTEM_YCM to ON, to ensure that even if there are errors gym-ignition never tries to install its own YCM.
Clearly, the main drawback of this approach is that the complexity of the build system increases.

See robotology/idyntree#322 and robotology/gazebo-fmi#23 for some related discussions.

[diegoferigo]

I was sure I could use your feedback to extend the minimal example I provided. Thanks for the insights. You caught many of the pitfalls of such system, and particularly your conclusion:

Clearly, the main drawback of this approach is that the complexity of the build system increases.

can be atomated by a new macro / function. It would be a great addition to this use-case for dependencies.

[claudiofantacci]

If instead you ask for a minimum version of YCM in your CMakeLists, and the system YCM is not enough, that is even worse: gym-ignition will install its own YCM, overwriting the system YCM if it is installed in the same prefix were gym-ignition is installed.

Well, unless you use the fetched content only locally for the sake of the build (which in turn may cause other problem, like interdependencies with other libraries, but this would happen anyway). Am I wrong?

[traversaro]

Well, unless you use the fetched content only locally for the sake of the build (which in turn may cause other problem, like interdependencies with other libraries, but this would happen anyway). Am I wrong?

You are indeed perfectly right, my error is that I taught that this line
https://github.com/robotology/gym-ignition/blob/44098f150cdad9031a2890cb705ba7cd7523e474/cmake/BootstrapYCM.cmake#L14
actually was

add_subdirectory(${ycm_SOURCE_DIR})

in that case, indeed there is no problem related to the installation.
However, even for just a "private use" having the possibility of explicitly disabling the use of vendored libraries is useful for packaging.

[diegoferigo]

@traversaro YCM is a special case, and the gym-ignition approach only prevents vendoring files (removing the burden to manually update them by time to time). However, what you described in your comment above is completely true in the case that (i) you use add_subdirectory and (ii) the project has install directives (e.g. the other gym-ignition dependency, tiny-process-library, disable install support if it is a subproject, and it is up to the user installing its files if needed).

[claudiofantacci]

However, even for just a "private use" having the possibility of explicitly disabling the use of vendored libraries is useful for packaging.

I totally agree here 👍

Just out of curiosity, what is your opinion about the following scenario: a required version of a library is not found on the system. The user allows for external fetched dependencies and, to avoid corrupting the system with installations, the subject dependency is fetched, compiled and used as a static library, hence just for the sake of building the project. This would probably avoid some trouble and overall the procedure is constrained to the project build. I fear, however, that I'm missing something important 🤔

[diegoferigo]

@claudiofantacci In my opinion this is legit if the dependency is PRIVATE and third-party projects that want to import your project do not need to directly find it. Of course, problems might arise if the public interface of your project requires either including dependencies headers or linking against the dependency library.

[claudiofantacci]

@claudiofantacci In my opinion this is legit if the dependency is PRIVATE and third-party projects that want to import your project do not need to directly find it. Of course, problems might arise if the public interface of your project requires either including dependencies headers or linking against the dependency library.

Sure PRIVATE only. My bad not having specified that 👍
Thanks for the feedback!

[traversaro]

Just out of curiosity, what is your opinion about the following scenario: a required version of a library is not found on the system. The user allows for external fetched dependencies and, to avoid corrupting the system with installations, the subject dependency is fetched, compiled and used as a static library, hence just for the sake of building the project. This would probably avoid some trouble and overall the procedure is constrained to the project build. I fear, however, that I'm missing something important 🤔

I guess this is ok, but if your project is itself a library, I would recommend to make sure that you properly rename/hide the symbols of the vendored internal static library, to avoid that your project is used with something else that uses another version of the library that you vendor, leading to symbol clashes. These symbol clashes can lead to runtime problems such as the one discussed in simbody/simbody#510 and osqp/osqp#85 . Unfortunately, if you do not want to modify the vendored library, the details on the symbol hiding are platform specific.

[claudiofantacci]

Thanks @traversaro, great insights 👍