-
Notifications
You must be signed in to change notification settings - Fork 251
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
Update to use yaml-cpp version 0.8.0. #1605
Conversation
In yaml-cpp 0.8.0, they updated to use a proper CMake target, i.e. yaml-cpp::yaml-cpp. Update that here as well since we are updating our vendor to 0.8.0. While I was in here, I did a bunch of cleanup to make fewer symbols externally visible. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clalancette Thanks for cleanups. LGTM with green CI.
Hi, this is causing my builds to fail with the error:
Pulling from the commit before this one resolves this issue. I noticed that the automated builds are also failing with this issue? |
Yeah, that will be the case until we do releases of all 4 of the connected PRs. Given that we are just 2.5 working days from the freeze, we'll probably wait until Tuesday, April 16 to do that. Until then, you'll have to build this along with ros2/yaml_cpp_vendor#48 |
Ah, that makes sense. Sorry, was not aware of how ROS handles releases! |
This reverts commit 86be6e7.
This reverts commit 86be6e7
In yaml-cpp 0.8.0, they updated to use a proper CMake target, i.e. yaml-cpp::yaml-cpp. Update that here as well since we are updating our vendor to 0.8.0.
While I was in here, I did a bunch of cleanup to make fewer symbols externally visible.
Let me know if you want me to split the cleanups out into a separate PR; I'm happy to do that.
Depends on ros2/yaml_cpp_vendor#48 , so should not be merged before that one.