Skip to content

Fix linking issues for ODE on macOS #549

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.

Sign up for GitHub

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

Merged
tylerjw merged 11 commits into from
Jul 30, 2021
Merged

Fix linking issues for ODE on macOS #549

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dd33b8b
fixes for bullet
nkalupahana Jul 8, 2021
525d00c
fix ode linking issue
nkalupahana Jul 9, 2021
5dd3139
remove findbullet
nkalupahana Jul 9, 2021
1e157e9
Merge branch 'main' into main
nkalupahana Jul 9, 2021
c372376
Merge branch 'main' into main
nkalupahana Jul 12, 2021
b6e8476
Merge branch 'main' into main
nkalupahana Jul 20, 2021
a938c58
Merge branch 'main' into main
nkalupahana Jul 27, 2021
e974e19
Merge branch 'main' into main
nkalupahana Jul 29, 2021
69d12b0
Merge branch 'main' into main
nkalupahana Jul 30, 2021
171cdd4
Update CMakeLists.txt
nkalupahana Jul 30, 2021
dae2470
Merge branch 'main' into main
henningkayser Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion moveit_core/CMakeModules/FindBULLET.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ if(PKGCONFIG_FOUND)
endif()

find_package_handle_standard_args(BULLET
REQUIRED_VARS BULLET_LIBRARIES BULLET_INCLUDE_DIRS
REQUIRED_VARS BULLET_LIBRARIES BULLET_INCLUDE_DIRS BULLET_LIBRARY_DIRS
VERSION_VAR BULLET_VERSION)
4 changes: 4 additions & 0 deletions moveit_core/collision_detection_bullet/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Copy link
Copy Markdown
Member

@henningkayser henningkayser Jul 9, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nkalupahana nkalupahana Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

endif()

ament_target_dependencies(${MOVEIT_LIB_NAME}
rclcpp
rmw_implementation
Expand Down
5 changes: 5 additions & 0 deletions moveit_planners/ompl/ompl_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Copy link
Copy Markdown
Member

@henningkayser henningkayser Jul 9, 2021

Choose a reason for hiding this comment

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

Same here. Is ODE not exported correctly?

Copy link
Copy Markdown
Contributor Author

@nkalupahana nkalupahana Jul 9, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nkalupahana nkalupahana Jul 9, 2021

Choose a reason for hiding this comment

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

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.

endif()

ament_target_dependencies(${MOVEIT_LIB_NAME}
moveit_core
moveit_msgs
Expand Down