Removed the BULLET_ENABLE flag #489
Conversation
| @@ -128,9 +127,10 @@ set(THIS_PACKAGE_INCLUDE_DEPENDS | |||
| Eigen3 | |||
| eigen3_cmake_module | |||
| OCTOMAP | |||
| ${BULLET_ENABLE} | |||
There was a problem hiding this comment.
Bullet needs to be listed as include depend, no?
There was a problem hiding this comment.
I think I omitted it because I was mirroring the FCL setup. In fact the FCL include dependency is not in this list but I can add the Bullet one if that's what's preferred.
| @@ -25,29 +25,27 @@ target_link_libraries(${MOVEIT_LIB_NAME} | |||
| ${BULLET_LIBRARIES} | |||
There was a problem hiding this comment.
Can you add Bullet to the ament_target_dependencies block above? The variables seem to match the right pattern, so this should work.
There was a problem hiding this comment.
Isn't that only necessary for ros2 dependencies?
There was a problem hiding this comment.
Isn't that only necessary for ros2 dependencies?
no, it works for all kinds of packages as long as they provide the right variables (_LIBRARIES, _INCLUDE_DIRS, ...).
|
|
||
| install(DIRECTORY include/ DESTINATION include) | ||
| install(TARGETS ${MOVEIT_LIB_NAME} EXPORT ${MOVEIT_LIB_NAME} | ||
| TARGETS collision_detector_bullet_plugin EXPORT collision_detector_bullet_plugin | ||
| LIBRARY DESTINATION lib | ||
| ARCHIVE DESTINATION lib | ||
| RUNTIME DESTINATION bin |
There was a problem hiding this comment.
I think the runtime destination should be lib/${PROJECT_NAME}, otherwise all executables will be accessible in the global bin path. That's a change that we should probably apply for the whole codebase.
There was a problem hiding this comment.
OK, I can change this
There was a problem hiding this comment.
@jrgnicho I hope you don't mind me hijacking this PR. The missing piece was that the bullet includes weren't configured correctly. ament_target_dependencies does this implicitly. The SYSTEM keyword was added to silence build warnings in bullet headers. I tested this with the MoveItCpp demo and it worked just fine for me.
I'd wait with merging this until the ROS 1 sync that @JafarAbdi is preparing is ready and merged.
Codecov Report
@@ Coverage Diff @@
## main #489 +/- ##
==========================================
+ Coverage 45.18% 46.59% +1.42%
==========================================
Files 169 183 +14
Lines 18732 19657 +925
==========================================
+ Hits 8462 9158 +696
- Misses 10270 10499 +229
Continue to review full report at Codecov.
|
|
@henningkayser thanks for fixing that, do you need me to test anything else? |
|
Just an FYI, this PR breaks compilation on macOS. I'll bring in a fix when I have time to debug the issue (though @jrgnicho I would appreciate it if you could take a look at the CI logs to see if this is something you know how to fix off the bat). Obviously this isn't anyone's fault, since there's no macOS CI -- all it really shows is the need for it :) I'll make an issue for that later to track my progress. https://github.com/nkalupahana/ros2-foxy-macos/runs/2936771527?check_suite_focus=true |
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Description
Addresses issue #473
Removed the BULLET_ENABLE flag
Properly installs the bullet collision plugin
Checklist