Install moveit_core headers within additional moveit_core directory#1785
Install moveit_core headers within additional moveit_core directory#1785vatanaksoytezer merged 1 commit intomoveit:mainfrom ChrisThrasher:target_include_directories
Conversation
target_include_directoriestarget_include_directories and change where headers get installed
tylerjw
left a comment
There was a problem hiding this comment.
Awesome, thank you @ChrisThrasher.
Here is the link to the colcon docs that justify some of this change: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#how-to-make-it-easier-for-your-users-to-override
Codecov ReportBase: 50.90% // Head: 50.89% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1785 +/- ##
==========================================
- Coverage 50.90% 50.89% -0.00%
==========================================
Files 378 378
Lines 31728 31728
==========================================
- Hits 16148 16145 -3
- Misses 15580 15583 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
target_include_directories and change where headers get installed|
Looks good to me, thank you for doing this Chris! |
Description
I changed where headers get installed to fix issues with building MoveIt from source when another copy is already installed. By installing all headers inside a
moveit_coredirectory, we add an extra layer of indirection that ensures we know what headers are getting used at any given time.While I was at it, I also made sure to stop using
include_directoriesinside moveit_core. CMake best practices are to avoid usinginclude_directories, particularly in large projects like this because it makes it difficult to understand what targets have access to what include directories. Removing them from moveit_core makes it easier for us to know what headers we need to install plus it revealed an issue where kinematics_base and robot_model depended on each other. This makes me wonder if perhaps moveit_core should be consolidated into fewer targets.