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

map_server: update to yaml-cpp 0.8.0 #4267

Closed
wants to merge 1 commit into from
Closed

map_server: update to yaml-cpp 0.8.0 #4267

wants to merge 1 commit into from

Conversation

981213
Copy link
Contributor

@981213 981213 commented Apr 18, 2024

yaml-cpp 0.8.0 requires explicit linking against yaml-cpp::yaml-cpp.
This fixes building breakage on rolling due to ros2/yaml_cpp_vendor#48

Basic Info

Info Please fill out this column
Ticket(s) this addresses None
Primary OS tested on Ubuntu 24.04
Robotic platform tested on Not yet. It shouldn't affect anything pratically.
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Fix building on rolling due to recent yaml-cpp upgrade

Description of documentation updates required from your changes

None.

Future work that may be required in bullet points

None.

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@981213
map_server: update to yaml-cpp 0.8.0
944f0a0 
yaml-cpp 0.8.0 requires explicit linking against yaml-cpp::yaml-cpp.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 18, 2024

@981213, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@981213
Copy link
Contributor Author

981213 commented Apr 18, 2024

The CI failed to build because it doesn't have the latest yaml_cpp_vendor package.

@SteveMacenski
Copy link
Member

@981213 should this PR wait then until that lands?

SteveMacenski
SteveMacenski requested changes Apr 21, 2024
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment •
edited

Choose a reason for hiding this comment

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

Please follow up, I don't think this is an appropriate PR since we have the vendor. If the vendor needs to be updated, then update that and it'll be reflected here after a sync. Why are these changes needed here instead - that breaks the vendorization?

nav2_map_server/CMakeLists.txt
@@ -11,6 +11,7 @@ find_package(rclcpp_components REQUIRED)
find_package(nav_msgs REQUIRED)
find_package(nav2_msgs REQUIRED)
find_package(yaml_cpp_vendor REQUIRED)
find_package(yaml-cpp REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

This should not be necessary, this is the role of the vendor package.

nav2_map_server/CMakeLists.txt
@@ -118,7 +119,8 @@ target_include_directories(${map_io_library_name} SYSTEM PRIVATE
${GRAPHICSMAGICKCPP_INCLUDE_DIRS})

target_link_libraries(${map_io_library_name}
${GRAPHICSMAGICKCPP_LIBRARIES})
${GRAPHICSMAGICKCPP_LIBRARIES}
yaml-cpp::yaml-cpp)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto and below

@981213
Copy link
Contributor Author

981213 commented Apr 22, 2024

should this PR wait then until that lands?

yaml_cpp_vendor 9.0.0 has been released into rolling, and that's how I noticed this problem. This is caused by the _vendor package update and independent of OS.

Why are these changes needed here instead - that breaks the vendorization?

This PR is created according to
ros2/yaml_cpp_vendor#48 (comment)
and following:
ros-perception/image_common#305
ros2/rviz#1183
ros2/rosbag2#1605

The solution provided upstream is to change all these downstream.

@981213 981213 mentioned this pull request Apr 22, 2024
Merged
@981213
Copy link
Contributor Author

981213 commented Apr 22, 2024

I agree that this change is a bit strange. Let me ask in that yaml-cpp update PR.

@SteveMacenski
Copy link
Member

Build has a number of complaints along the lines of

CMake Error at CMakeLists.txt:32 (add_executable):
  Target "map_server" links to target "yaml-cpp::yaml-cpp" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

@981213
Copy link
Contributor Author

981213 commented Apr 23, 2024

Oh, yaml_cpp_vendor 9.0 isn't available as binary package for jammy:

sudo apt-cache madison ros-rolling-yaml-cpp-vendor
ros-rolling-yaml-cpp-vendor | 8.3.1-1jammy.20240124.152723 | http://mirrors.ustc.edu.cn/ros2/ubuntu jammy/main amd64 Packages

sudo apt-cache madison ros-rolling-yaml-cpp-vendor
ros-rolling-yaml-cpp-vendor | 9.0.0-1noble.20240416.181051 | http://packages.ros.org/ros2/ubuntu noble/main amd64 Packages

This PR should be blocked until the switching to noble then.

@SteveMacenski
Copy link
Member

OK! I'll tag it that way with the 24.04 PR, thanks for the help and proactivity!

@SteveMacenski
Copy link
Member

Note to self that this is also a problem with Robot Localization to fix

@981213
Copy link
Contributor Author

981213 commented Apr 24, 2024

Note to self that this is also a problem with Robot Localization to fix

It's already fixed there actually: cra-ros-pkg/robot_localization@1ef1a57

@azeey azeey mentioned this pull request Apr 25, 2024
Merged
@tonynajjar
Copy link
Contributor

I ran into the same issue and sunk 2 hours to fix it in a similar way,if only I found this PR beforehand 😅 Anyway, I think this should now be ready to be merged

@tonynajjar tonynajjar mentioned this pull request May 6, 2024
Merged
7 tasks
@SteveMacenski
Copy link
Member

https://app.circleci.com/pipelines/github/ros-planning/navigation2/11466/workflows/31511139-b085-4f4b-ad71-5573452fa375/jobs/35332/steps

CI tells me this shouldn't happen until the 24.04 migration

@tonynajjar
Copy link
Contributor

Ah I tried it with the rolling docker images and there it was needed to work. I guess CI is not using latest rolling yet

@hidmic hidmic mentioned this pull request May 9, 2024
Merged
7 tasks
@aosmw aosmw mentioned this pull request May 14, 2024
Open
@SteveMacenski
Copy link
Member

@981213 thanks for the contribution here - @tonynajjar is working on the full 24.04 migration which this commit was added to in #4298. So, I'm closing this as that PR supersedes with some other necessary changes. I want to thank you for your help here and responses! It is greatly appreciated - it all takes a village :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees
No one assigned
Labels
blocked-24
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@981213 @SteveMacenski @tonynajjar