Skip to content

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

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 <[email protected]>
@sjahr sjahr requested a review from ChrisThrasher August 12, 2023 15:10
@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
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 <[email protected]>
(cherry picked from commit 4c901b2)

* Add PUBLIC/PRIVATE keywords

Signed-off-by: Shane Loretz <[email protected]>
(cherry picked from commit 68a6ee2)

---------

Co-authored-by: Shane Loretz <[email protected]>
henningkayser pushed a commit that referenced this pull request Aug 15, 2023
@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.

4 participants