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.

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

Jump to bottom

Fix installation of moveit_ros_control_interface header files #789

Merged
merged 6 commits into from
Nov 15, 2021
Merged

Fix installation of moveit_ros_control_interface header files #789

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
13e50b4
fix installed path of control interface header files
schornakj Nov 9, 2021
fd22eab
delete duplicated header installation
schornakj Nov 10, 2021
672a4ad
Revert "delete duplicated header installation"
schornakj Nov 11, 2021
be4e27a
Merge branch 'main' into pr-fix-controller-handle-header-install
tylerjw Nov 12, 2021
5d44c90
Merge branch 'main' into pr-fix-controller-handle-header-install
tylerjw Nov 15, 2021
88fc078
Merge branch 'main' into pr-fix-controller-handle-header-install
schornakj Nov 15, 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
3 changes: 2 additions & 1 deletion moveit_plugins/moveit_ros_control_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ EXPORT export_${PROJECT_NAME}
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
INCLUDES DESTINATION include
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this .?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to get the expected result without that, but I haven't dug into why exactly that is yet.

Copy link
Contributor Author

@schornakj schornakj Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I checked this again, and it looks like it's actually the other way around. The install function needs to have the INCLUDES DESTINATION include line for the headers to be found by other packages, so the line here is needed but the other one can be deleted. (I assume this has something to do with exporting information about the library headers in CMake).

Copy link
Member

Choose a reason for hiding this comment

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

afaik, this doesn't really install the headers but it only makes them available for the target. This should not be necessary, if the headers are actually installed in the right path, adding this is line is still correct.

)

## Mark cpp header files for installation
install(DIRECTORY include/${PROJECT_NAME}/
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we would use install(DIRECTORY include/ DESTINATION include). Otherwise, the headers will not be inside the projects path when you call #include <project_name/header.h>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll revert the change that deleted this line and push up again. I didn't do a clean build for this package when I was testing my change, which I realized means that changes to what was installed weren't reflected by the state of the built workspace.

install(DIRECTORY include/
DESTINATION include
)

Expand Down