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

Remove YAML_CPP_DLL define #964

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Remove YAML_CPP_DLL define #964

merged 1 commit into from
Oct 4, 2022

Conversation

Ace314159
Copy link
Contributor

Since ros2/yaml_cpp_vendor#10 is fixed, the explicit YAML_CPP_DLL define is no longer needed and needs to be removed to prevent the macro redefinition warnings.

This should be backported to foxy and galactic.

@Ace314159 Ace314159 requested a review from a team as a code owner February 19, 2022 17:31
@Ace314159 Ace314159 requested review from gbiggs and lihui815 and removed request for a team February 19, 2022 17:31
Signed-off-by: Akash Munagala <akash.munagala@gmail.com>
@gbiggs
Copy link
Member

gbiggs commented Feb 20, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Ace314159
Copy link
Contributor Author

Ace314159 commented Feb 21, 2022

Perhaps ros2/yaml_cpp_vendor#10 wasn't fixed? It's interesting that ros-perception/image_common#231 seems to be fine though.

@gbiggs
Copy link
Member

gbiggs commented Feb 21, 2022

Have you successfully built it on Windows locally?

@jacobperron
Copy link
Member

With respect to how yaml_cpp_vendor is being linked against, the only difference I can spot between rosbag2_storage and the other two packages in rviz and image_common is that the other two package specify the link dependency as PUBLIC (for example). Maybe we could try doing the same here:

ament_target_dependencies(
${PROJECT_NAME}
pluginlib
rcpputils
rcutils
yaml_cpp_vendor)

I don't know if this will fix the build failure here, but it might fix a loosely related issue downstream: https://build.ros2.org/view/Rbin_rhel_el864/job/Rbin_rhel_el864__domain_bridge__rhel_8_x86_64__binary/107/consoleFull

23:06:51 DEBUG: /usr/bin/ld: warning: libyaml-cpp.so.0.7, needed by /opt/ros/rolling/lib/librosbag2_cpp.so, not found (try using -rpath or -rpath-link)
23:06:51 DEBUG: /opt/ros/rolling/lib/librosbag2_storage.so: undefined reference to `YAML::detail::node_data::empty_scalar[abi:cxx11]()'
23:06:51 DEBUG: /opt/ros/rolling/lib/librosbag2_storage.so: undefined reference to `YAML::detail::node_data::convert_to_map(std::shared_ptr<YAML::detail::memory_holder> const&)'
23:06:51 DEBUG: /opt/ros/rolling/lib/librosbag2_storage.so: undefined reference to `YAML::detail::node::m_amount'
23:06:51 DEBUG: /opt/ros/rolling/lib/librosbag2_storage.so: undefined reference to `YAML::detail::node_data::push_back(YAML::detail::node&, std::shared_ptr<YAML::detail::memory_holder> const&)'

@jacobperron
Copy link
Member

Nevermind, rviz is also failing to build: https://ci.ros2.org/job/ci_windows/16574

@Ace314159
Copy link
Contributor Author

What did yaml-cpp 0.7.0 add from previous versions to provide the YAML_CPP_DLL define? I was looking at the source, and I wasn't able to find anything.

I don't think the linking needs to be changed since the galactic and foxy fixes for yaml_cpp_vendor have the macro redefinition warnings without any changes here.

@Ace314159
Copy link
Contributor Author

Should we try applying the change from ros2/yaml_cpp_vendor#31 to the main branch of yaml_cpp_vendor since we know that change does define YAML_CPP_DLL

@jacobperron
Copy link
Member

I've ported the yaml_cpp_vendor change to export YAML_CPP_DLL to Rolling (ros2/yaml_cpp_vendor#38) and triggered CI, but there's still an issue with rosbag2_storage:

  • Windows Build Status

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@Ace314159
Copy link
Contributor Author

Ace314159 commented Sep 27, 2022

@jacobperron I've identified the cause of the build failure. I've suggested a fix on the yaml_cpp_vendor PR.

@jacobperron
Copy link
Member

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants