Compilation Fixes for MoveIt on macOS #498
Conversation
TODO: stop conflict in glutGetProcAddress between freeglut and glut
| set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES COMPILE_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}") | ||
| set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES LINK_FLAGS "${OpenMP_CXX_FLAGS}") | ||
| if(APPLE) | ||
| target_link_libraries(${MOVEIT_LIB_NAME} OpenMP::OpenMP_CXX) |
There was a problem hiding this comment.
Did you try setting OpenMP with ament_target_dependencies?
There was a problem hiding this comment.
Unfortunately, using ament_target_dependencies(${MOVEIT_LIB_NAME} OpenMP) leads to a linker error. https://github.com/nkalupahana/ros2-foxy-macos/runs/2841920686?check_suite_focus=true
target_link_libraries seems to be necessary. Is that okay?
There was a problem hiding this comment.
@henningkayser ament_target_dependencies and target_link_libraries do two different things. ament_target_dependencies adds those dependencies to the list that gets set in the export target, where-as target_link_libraries configures linking. In this case we might need both, I'm not sure.
There was a problem hiding this comment.
@tylerjw ament_target_dependencies calls target_link_libraries internally and also configures include directories and other compile definitions. A well configured package should always work with ament_target_dependencies if the variables follow conventions. See https://github.com/ament/ament_cmake/blob/master/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake#L21
There was a problem hiding this comment.
it's fine if OpenMP doesn't work here, though
| set(GLUT_LIBS GLUT::GLUT) | ||
| endif() | ||
| set(perception_GL_INCLUDE_DIRS "mesh_filter/include" "depth_image_octomap_updater/include") | ||
| set(SYSTEM_GL_INCLUDE_DIRS ${GLEW_INCLUDE_DIR} ${GLUT_INCLUDE_DIR}) |
There was a problem hiding this comment.
If using ament_target_dependencies doesn't work, I'd suggest adding a SYSTEM_GL_LIBRARIES variable like this one.
There was a problem hiding this comment.
Alright, did that!
|
@henningkayser fixes are in, please approve the workflows when you get the chance :) |
Codecov Report
@@ Coverage Diff @@
## main #498 +/- ##
==========================================
- Coverage 45.25% 45.23% -0.01%
==========================================
Files 169 169
Lines 18723 18723
==========================================
- Hits 8471 8468 -3
- Misses 10252 10255 +3
Continue to review full report at Codecov.
|
|
@henningkayser can we get this merged in? I've addressed the concerns from your review where possible. |
|
I approved and started running the CI workflows for this. Would you mind rebasing this on main so we can get it merged (assuming CI passes)? |
|
@tylerjw done! You'll have to re-approve the CI, though. |
* Fixed location of GLUT,GLEW framework on macOS * Use explicit boost namespace for bind placeholders
3 participants
Description
This pull request fixes various compilation issues on macOS, allowing nearly all of Moveit2 to be compiled on macOS. This build has been verified on CI for macOS 10.15: https://github.com/nkalupahana/ros2-foxy-macos/runs/2815266613?check_suite_focus=true
Checklist