Fix MoveIt Servo compilation on macOS #555

nkalupahana · 2021-07-13T06:07:56Z

Description

This PR fixes MoveIt Servo compilation on macOS. Closes #491
Broken CI: https://github.com/nkalupahana/ros2-foxy-macos/runs/3026534213?check_suite_focus=true
Now working: https://github.com/nkalupahana/ros2-foxy-macos/runs/3053981174?check_suite_focus=true

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

codecov · 2021-07-13T07:05:37Z

Codecov Report

Merging #555 (09c2338) into main (64535cb) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
+ Coverage   46.71%   46.73%   +0.02%     
==========================================
  Files         184      184              
  Lines       19686    19686              
==========================================
+ Hits         9195     9198       +3     
+ Misses      10491    10488       -3

Impacted Files	Coverage Δ
...veit_servo/include/moveit_servo/servo_parameters.h	`0.00% <ø> (ø)`
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`75.85% <0.00%> (+1.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64535cb...09c2338. Read the comment docs.

JafarAbdi · 2021-07-13T10:07:05Z

moveit_ros/moveit_servo/CMakeLists.txt

+add_library(${POSE_TRACKING} SHARED src/pose_tracking.cpp src/servo_parameters.cpp)
 ament_target_dependencies(${POSE_TRACKING} ${THIS_PACKAGE_INCLUDE_DEPENDS})
 target_link_libraries(${POSE_TRACKING} ${SERVO_LIB_NAME})


I think we should link it against the already built library rather than adding the source files to the target

Suggested change

add_library(${POSE_TRACKING} SHARED src/pose_tracking.cpp src/servo_parameters.cpp)

ament_target_dependencies(${POSE_TRACKING} ${THIS_PACKAGE_INCLUDE_DEPENDS})

target_link_libraries(${POSE_TRACKING} ${SERVO_LIB_NAME})

add_library(${POSE_TRACKING} SHARED src/pose_tracking.cpp)

ament_target_dependencies(${POSE_TRACKING} ${THIS_PACKAGE_INCLUDE_DEPENDS})

target_link_libraries(${POSE_TRACKING} ${SERVO_LIB_NAME} ${SERVO_PARAM_LIB_NAME})

You can't link against the already-built library, because as you can see from the failing CI, this is a template issue -- the already-built library doesn't have the correct templated classes built in, which is why linker fails. Thus, the source has to be built each time (which is what my change does) to allow for any additional template classes that need to be built.

If this isn't okay, I think including the implementation (cpp) in the .h file would also work; that's the pattern I've seen for template classes in the past, at least. Let me know what you want to see.

Failing CI of this change: https://github.com/nkalupahana/ros2-foxy-macos/runs/3058543555?check_suite_focus=true

I think including the implementation (cpp) in the .h file would also work; that's the pattern I've seen for template classes in the past, at least. Let me know what you want to see.

Yes, I think this should be the solution

Alright, so it looks like you can't just include the .cpp file because not everything in there is templated so if you include it, then you'll get duplicate symbols. To fix that, I just moved the template function into the .h file (the forward declaration is already in there, so I figured that wouldn't be too big of a deal). Let me know if this works!

CI: https://github.com/nkalupahana/ros2-foxy-macos/actions/runs/1027725851

I'd like to recommend the rationale for this be added as a comment to this CMake file... just copy-paste your explanation here, into the code :-)

This reverts commit afa8c8d.

JafarAbdi · 2021-07-13T20:15:27Z

moveit_ros/moveit_servo/include/moveit_servo/servo_parameters.h

+// Must be declared here to ensure template class is built for required templates when included
+template <typename T>
+void declareOrGetParam(T& output_value, const std::string& param_name, const rclcpp::Node::SharedPtr& node,
+                       const rclcpp::Logger& logger)


Why did you remove the default value parameter .?

Oh sorry, not sure. I'll fix that.

Okay, I think this is good.

moveit_ros/moveit_servo/include/moveit_servo/servo_parameters.h

Co-authored-by: Jafar Abdi <[email protected]>

nkalupahana · 2021-07-13T20:25:31Z

@JafarAbdi any idea what's going on with CI? I don't think that failure is due to this PR.

JafarAbdi · 2021-07-13T20:26:58Z

@JafarAbdi any idea what's going on with CI? I don't think that failure is due to this PR.

Look like the build farm is broken for rolling, I'll retrigger the workflow after few hours to see if it got fixed

nkalupahana · 2021-07-15T00:52:24Z

@JafarAbdi looks like a slightly different CI issue, has this been reported elsewhere? I didn't see any issues for it on ros2_control.

nkalupahana · 2021-07-24T17:20:19Z

@JafarAbdi rolling CI passed on 42ba2fa, but now it's broken again? Not sure what's going on. If it's okay with you, I think we should just merge this in -- rolling CI has now passed, and these breaks in the CI now have nothing to do with the PR :)

davetcoleman · 2021-07-27T17:59:08Z

@tylerjw agrees we can merge this because rolling is broken...

tylerjw

This does not change the functionality and will be part of the code I'll be replacing with my parameters refactor. I'll merge this without rolling-testing passing (it is a ros infrastructure problem).

* Update CMakeLists.txt * Revert "Update CMakeLists.txt" This reverts commit afa8c8d. * move template function to .h * re-added default_value * Update moveit_ros/moveit_servo/include/moveit_servo/servo_parameters.h Co-authored-by: Jafar Abdi <[email protected]> * rerun CI * rerun CI * rerun CI * rerun CI * added note about duplicate symbols Co-authored-by: Jafar Abdi <[email protected]> Co-authored-by: Tyler Weaver <[email protected]>

Update CMakeLists.txt

afa8c8d

JafarAbdi reviewed Jul 13, 2021

View reviewed changes

nkalupahana requested a review from JafarAbdi July 13, 2021 16:50

nkalupahana added 2 commits July 13, 2021 13:11

Revert "Update CMakeLists.txt"

fc63712

This reverts commit afa8c8d.

move template function to .h

515ec5e

JafarAbdi reviewed Jul 13, 2021

View reviewed changes

re-added default_value

d94990f

JafarAbdi reviewed Jul 13, 2021

View reviewed changes

moveit_ros/moveit_servo/include/moveit_servo/servo_parameters.h Outdated Show resolved Hide resolved

Update moveit_ros/moveit_servo/include/moveit_servo/servo_parameters.h

7bde959

Co-authored-by: Jafar Abdi <[email protected]>

nkalupahana added 2 commits July 13, 2021 22:44

rerun CI

49fd073

rerun CI

ced3203

nkalupahana and others added 2 commits July 15, 2021 21:51

rerun CI

0045292

Merge branch 'main' into patch-1

018a167

AndyZe approved these changes Jul 16, 2021

View reviewed changes

nkalupahana added 2 commits July 21, 2021 14:38

rerun CI

42ba2fa

Merge branch 'main' into patch-1

3e07e2c

nkalupahana mentioned this pull request Jul 26, 2021

CI is broken for rolling #557

Closed

tylerjw and others added 2 commits July 27, 2021 11:59

Merge branch 'main' into patch-1

979f1e0

added note about duplicate symbols

09c2338

tylerjw approved these changes Jul 27, 2021

View reviewed changes

tylerjw merged commit cf5fb50 into moveit:main Jul 27, 2021

nkalupahana deleted the patch-1 branch July 27, 2021 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MoveIt Servo compilation on macOS #555

Fix MoveIt Servo compilation on macOS #555

nkalupahana commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

JafarAbdi Jul 13, 2021

nkalupahana Jul 13, 2021 •

edited

Loading

JafarAbdi Jul 13, 2021

nkalupahana Jul 13, 2021

davetcoleman Jul 27, 2021

nkalupahana Jul 27, 2021

JafarAbdi Jul 13, 2021

nkalupahana Jul 13, 2021

nkalupahana Jul 13, 2021

nkalupahana commented Jul 13, 2021

JafarAbdi commented Jul 13, 2021

nkalupahana commented Jul 15, 2021

nkalupahana commented Jul 24, 2021

davetcoleman commented Jul 27, 2021

tylerjw left a comment

Fix MoveIt Servo compilation on macOS #555

Fix MoveIt Servo compilation on macOS #555

Conversation

nkalupahana commented Jul 13, 2021 • edited Loading

Description

Checklist

codecov bot commented Jul 13, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nkalupahana Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkalupahana commented Jul 13, 2021

JafarAbdi commented Jul 13, 2021

nkalupahana commented Jul 15, 2021

nkalupahana commented Jul 24, 2021

davetcoleman commented Jul 27, 2021

tylerjw left a comment

Choose a reason for hiding this comment

nkalupahana commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

nkalupahana Jul 13, 2021 •

edited

Loading