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.

Sign up for GitHub

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

Jump to bottom

Upgrade to yaml-cpp 0.7.0 #25

Merged
merged 2 commits into from
Feb 2, 2022
Merged

Upgrade to yaml-cpp 0.7.0 #25

merged 2 commits into from
Feb 2, 2022

Conversation

clalancette
Copy link
Contributor

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

@clalancette
Copy link
Contributor Author

Assuming this works, this should supersede #23, and should also solve #10.

@clalancette clalancette mentioned this pull request Oct 22, 2021
Closed
@clalancette
Copy link
Contributor Author

clalancette commented Oct 22, 2021 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows Debug Build Status

@clalancette
Copy link
Contributor Author

Another try with a fix for Linux:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows Debug Build Status

@clalancette clalancette force-pushed the clalancette/yaml-cpp-0.7.0 branch from bb975d9 to d2dc136 Compare January 5, 2022 21:01
@clalancette
Copy link
Contributor Author

Rebased on the latest. Here is another CI try:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows Debug Build Status

@Ace314159 Ace314159 mentioned this pull request Jan 23, 2022
Closed
@clalancette
Upgrade to yaml-cpp 0.7.0
33edbf9 
This includes a fix for macOS/Linux where newer yaml-cpp
moved the cmake files from
$prefix/opt/yaml_cpp_vendor/lib/cmake/yaml-cpp into
$prefix/opt/yaml_cpp_vendor/share/cmake/yaml-cpp

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/yaml-cpp-0.7.0 branch from d2dc136 to 33edbf9 Compare January 27, 2022 21:18
@clalancette clalancette marked this pull request as ready for review January 27, 2022 21:18
@clalancette
Copy link
Contributor Author

clalancette commented Jan 27, 2022 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • Linux Jammy Build Status

@j-rivero
Copy link

There is one more 0.6 reference in https://github.com/ros2/yaml_cpp_vendor/pull/25/files#diff-b5b35010854e957d6bd0c2b492e798ff01b5265c02244afa4f09fc7665a1adf2L1. Not sure if that was leaved out in propose.

A part from that, changes looks good to me (Jammy warnings seems to me totally unrelated to the PR).

@clalancette
Copy link
Contributor Author

There is one more 0.6 reference in https://github.com/ros2/yaml_cpp_vendor/pull/25/files#diff-b5b35010854e957d6bd0c2b492e798ff01b5265c02244afa4f09fc7665a1adf2L1. Not sure if that was leaved out in propose.

Nope, I totally missed it. Thanks for pointing it out! I'll fix it.

@clalancette
Remove a specific version of yaml-cpp in the extras file.
0dc320d 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
wjwwood
wjwwood approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one question

CMakeLists.txt Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

Thanks for the reviews! I'm going to go ahead and merge this one.

@clalancette clalancette merged commit 21d51b7 into master Feb 2, 2022
@clalancette clalancette deleted the clalancette/yaml-cpp-0.7.0 branch February 2, 2022 15:53
@Ace314159
Copy link
Contributor

Will this be backported to foxy and galactic in order to fix #10 there as well?

@clalancette
Copy link
Contributor Author

Will this be backported to foxy and galactic in order to fix #10 there as well?

No, this is too big of a change for Foxy and Galactic. If we really want to fix this on Foxy and Galactic, we should do something like #29.

@Ace314159
Copy link
Contributor

I see, thanks for explaining! I made #30 and #31 for foxy and galactic.

@clalancette clalancette mentioned this pull request Feb 3, 2022
Closed
@clalancette clalancette mentioned this pull request Feb 18, 2022
Closed
@Ace314159 Ace314159 mentioned this pull request Sep 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich left review comments

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@clalancette @j-rivero @Ace314159 @wjwwood @nuclearsandwich