Fix linking issues for ODE on macOS #549
Conversation
Codecov Report
@@ Coverage Diff @@
## main #549 +/- ##
=======================================
Coverage 46.79% 46.79%
=======================================
Files 185 185
Lines 19693 19693
=======================================
Hits 9214 9214
Misses 10479 10479 Continue to review full report at Codecov.
|
| @@ -13,6 +13,10 @@ set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION "${${PROJECT_NAME}_V | |||
| ament_target_dependencies(${MOVEIT_LIB_NAME} SYSTEM | |||
| BULLET | |||
| ) | |||
| if(APPLE) | |||
| target_link_directories(${MOVEIT_LIB_NAME} PUBLIC ${BULLET_LIBRARY_DIRS}) | |||
There was a problem hiding this comment.
ament_target_dependencies should already search for the correct library dirs. (see here). Or is it actually missing the PUBLIC keyword. I didn't check thoroughly, but Bullet doesn't seem to be part of the public API.
There was a problem hiding this comment.
I believe this is an issue with the custom FindBULLET. Removing it fixed this issue. Let me know if that's okay -- it is quite old, and I think if the CI passes, it should be.
macOS CI: https://github.com/nkalupahana/ros2-foxy-macos/runs/3031358497?check_suite_focus=true
| @@ -26,6 +26,11 @@ set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION "${${PROJECT_NAME}_V | |||
|
|
|||
| find_package(OpenMP REQUIRED) | |||
|
|
|||
| # Used to link in ODE, an OMPL dependency, on macOS | |||
| if(APPLE) | |||
| target_link_directories(${MOVEIT_LIB_NAME} PUBLIC ${OMPL_LIBRARY_DIRS}) | |||
There was a problem hiding this comment.
Same here. Is ODE not exported correctly?
There was a problem hiding this comment.
Looking back at this, I think the issue might be that in moveit_planners/ompl/CMakeLists.txt, ompl is lowercase, but the variable is OMPL_LIBRARY_DIRS -- the code you linked to specifically say that case matters. I'll try it out.
There was a problem hiding this comment.
Looks like this doesn't work -- I have a feeling ODE just isn't exported correctly, so this is required. Let me know if there's something specific you want to try.
|
@henningkayser Let me know if you want any more changes, or if this is good to go! |
|
@henningkayser sorry to keep bothering you on this :) I'd like to get this merged in soon if there are no further issues! |
nkalupahana
changed the title
nkalupahana
changed the title
|
@nkalupahana do you know why it isn't passing for foxy in CI? |
|
@tylerjw It looks like CI is trying to use the ros2 branch instead of the foxy branch of geometric_shapes, which very recently got split off (moveit/geometric_shapes#200). I don't believe this has anything to do with this PR. |
|
You are correct. I'm working on a fix for that. Does this PR fix your issue with building on OSX? I couldn't tell based on the conversation thread above if this worked for you. |
|
Here is what I'm doing about foxy: #565 I hope to merge that soon and then we can update this based on that and I'm happy to merge this (as you say it fixes your issue and it doesn't seem to harm anything else). |
|
@nkalupahana sorry for letting this get stale. The change looks fine to me, I filed a related issue at ompl. lgtm after CI succeeds |
Co-authored-by: Henning Kayser <[email protected]>
Co-authored-by: Henning Kayser <[email protected]>
Description
This fixes the Bullet linking issues on macOS introduced by #489 (https://github.com/nkalupahana/ros2-foxy-macos/runs/3014655671?check_suite_focus=true). It also fixes an ODE linking issue that I've been using a temp fix for -- OMPL uses ODE, and this needs to be linked in (https://github.com/nkalupahana/ros2-foxy-macos/runs/3022396910?check_suite_focus=true).
(Bullet issue fixed by #530 so now this is just ODE)
Proof of build: https://github.com/nkalupahana/ros2-foxy-macos/runs/3198722057?check_suite_focus=true
Checklist