Skip to content

Upgrade to yaml-cpp 0.7.0#25

Merged
clalancette merged 2 commits intomasterfrom
clalancette/yaml-cpp-0.7.0
Feb 2, 2022
Merged

Upgrade to yaml-cpp 0.7.0#25
clalancette merged 2 commits intomasterfrom
clalancette/yaml-cpp-0.7.0

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

@clalancette clalancette commented Oct 22, 2021

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

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Oct 22, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

clalancette commented Oct 22, 2021

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
Copy Markdown
Contributor Author

clalancette commented Jan 5, 2022

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

nuclearsandwich
nuclearsandwich reviewed Jan 7, 2022
View reviewed changes
@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
Copy Markdown
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
Copy Markdown

j-rivero commented Jan 31, 2022

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
Copy Markdown
Contributor Author

clalancette commented Jan 31, 2022

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
Copy Markdown
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

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 2, 2022

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

Ace314159 commented Feb 3, 2022

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

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 3, 2022

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

Ace314159 commented Feb 3, 2022

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