Skip to content
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.

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

Install moveit_core headers within additional moveit_core directory #1785

Merged
merged 1 commit into from
Dec 6, 2022
Merged

Install moveit_core headers within additional moveit_core directory #1785

merged 1 commit into from
Dec 6, 2022

Conversation

ChrisThrasher
Copy link
Contributor

@ChrisThrasher ChrisThrasher commented Dec 5, 2022

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_core directory, 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_directories inside moveit_core. CMake best practices are to avoid using include_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.

@ChrisThrasher ChrisThrasher changed the title Use target_include_directories Use target_include_directories and change where headers get installed Dec 5, 2022
Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Base: 50.90% // Head: 50.89% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (267dc5e) compared to base (5846106).
Patch has no changes to coverable lines.

❗ Current head 267dc5e differs from pull request most recent head cc32e09. Consider uploading reports for the commit cc32e09 to get more accurate results

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     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.26% <0.00%> (-0.43%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.95% <0.00%> (+0.08%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ChrisThrasher ChrisThrasher changed the title Use target_include_directories and change where headers get installed Install moveit_core headers within additional moveit_core directory Dec 5, 2022
@vatanaksoytezer
Copy link
Contributor

Looks good to me, thank you for doing this Chris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants