search

ros2 / yaml_cpp_vendor

Vendor package for providing yaml cpp within a cmake package
0 stars 16 forks source link

NO_CMAKE_PACKAGE_REGISTRY causes problems with package managers #28

Open alsora opened 2 years ago

alsora commented 2 years ago

The presence of NO_CMAKE_PACKAGE_REGISTRY flag in https://github.com/ros2/yaml_cpp_vendor/blob/master/CMakeLists.txt#L71-L75 results in yaml-cpp to be searched only in system locations.

The comment mentions avoid finding the library downloaded in WORKSPACE B when building workspace A which I'm not sure I fully understand, however forcing everyone to either install yaml-cpp in the system or to download and build it in place does not seem a universally valid approach.

There are plenty of situations where yaml-cpp would be already available and it should be used (for example if a package manager such as conan is being used), rather than introducing conflicting dependencies in the system.

What do you think? Do we still need that flag? If yes, can we add a CMake variable to optionally disable it?

clalancette commented 2 years ago

So that change was originally part of a sequence of events. We first put in #8 to make things use the system installed yaml-cpp, which was reverted in #15, and then was re-applied with NO_CMAKE_PACKAGE_REGISTRY in #16. I think #16 has some of the breadcrumbs of why we applied the fix to begin with. @ivanpauno maybe you can shed some light on why we needed this fix, if you remember?

ivanpauno commented 2 years ago

IIRC, the issue is that was finding the package in the cmake registry, but it was causing issues at runtime because the directory was not added to LD_LIBRARY_PATH. See discussion in https://github.com/ros2/yaml_cpp_vendor/pull/16 for more details.

Currently the trick is that we add an enviroment hook when the package is being built https://github.com/ros2/yaml_cpp_vendor/blob/14b8539109eb7fa2ff4503d457bdfcd6f4ce9bf1/CMakeLists.txt#L79-L91. Maybe we should also create an environment hook if the package is found not in a system directory (?).

I'm not sure how to do that, but I guess that's possible. If that's fixed, I think we can remove the NO_CMAKE_PACKAGE_REGISTRY option.