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

Fix linking error with cached_ik_kinematics_plugin #2292

Merged
merged 4 commits into from Aug 14, 2023

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Aug 10, 2023

Description

This fixes a bug caused by #1677 which accidentally inverted the dependency between the plugin and the base library. This fixes it by making the plugin again depend on the base library, and adding missing dependencies to the base library.

I'm not sure why the base library is necessary. It seems like we could add ik_cache.cpp to the plugin library, but I fixed it this way because I would like to request this fix be backported to ROS Iron.

Steps to reproduce (sorry I didn't spend the time to make an MRE)

  1. Launch a moveit_ros_move_group move_group node with parameters that includes
robot_description_kinematics:
  some_group_name:
    kinematics_solver: cached_ik_kinematics_plugin/CachedKDLKinematicsPlugin
  1. Observe that it errors with:
symbol lookup error: /path/to/ws/install/moveit_kinematics/lib/libmoveit_cached_ik_kinematics_plugin.so: undefined symbol: _ZN27cached_ik_kinematics_plugin7IKCacheC1Ev

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks for this fix! I agree that the base library seems to be superfluous.

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (01ccced) 50.72% compared to head (449e7fe) 50.70%.

❗ Current head 449e7fe differs from pull request most recent head 9ce1be6. Consider uploading reports for the commit 9ce1be6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
- Coverage   50.72%   50.70%   -0.01%     
==========================================
  Files         386      386              
  Lines       31914    31914              
==========================================
- Hits        16185    16179       -6     
- Misses      15729    15735       +6     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjahr
Copy link
Contributor

sjahr commented Aug 11, 2023

Minor blocker for this: #2293

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
@sjahr sjahr added the backport-iron Mergify label that triggers a PR backport to Iron label Aug 14, 2023
Copy link
Contributor

@Abishalini Abishalini left a comment

Choose a reason for hiding this comment

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

Just tested this out. Thanks for fixing this

@Abishalini Abishalini merged commit d707d38 into moveit:main Aug 14, 2023
7 checks passed
sjahr pushed a commit that referenced this pull request Aug 15, 2023
…2300)

* Fix linking error with cached_ik_kinematics_plugin

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
(cherry picked from commit 4c901b2)

* Add PUBLIC/PRIVATE keywords

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
(cherry picked from commit 68a6ee2)

---------

Co-authored-by: Shane Loretz <sloretz@intrinsic.ai>
henningkayser pushed a commit that referenced this pull request Aug 15, 2023
Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
@sloretz sloretz deleted the sloretz__cache_ik_link_issues branch August 15, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants