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

NO_CMAKE_PACKAGE_REGISTRY causes problems with package managers #28

Open
alsora opened this issue Dec 11, 2021 · 2 comments
Open

NO_CMAKE_PACKAGE_REGISTRY causes problems with package managers #28

alsora opened this issue Dec 11, 2021 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@alsora
Copy link

alsora commented Dec 11, 2021

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
Copy link
Contributor

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 ivanpauno added enhancement New feature or request help wanted Extra attention is needed labels Dec 14, 2021
@ivanpauno
Copy link
Member

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 #16 for more details.

Currently the trick is that we add an enviroment hook when the package is being built

ament_environment_hooks(env_hook/yaml_cpp_vendor_library_path.bat)
set(ENV_VAR_NAME "PATH")
set(ENV_VAR_VALUE "opt\\yaml_cpp_vendor\\bin")
else()
ament_environment_hooks(env_hook/yaml_cpp_vendor_library_path.sh)
if(APPLE)
set(ENV_VAR_NAME "DYLD_LIBRARY_PATH")
else()
set(ENV_VAR_NAME "LD_LIBRARY_PATH")
endif()
set(ENV_VAR_VALUE "opt/yaml_cpp_vendor/lib")
endif()
ament_environment_hooks(env_hook/yaml_cpp_vendor_library_path.dsv.in)
.
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants