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

Fix installation of moveit_ros_control_interface header files #789

Merged

Conversation

schornakj
Copy link
Contributor

Description

The header files for the moveit_ros_control_interface::ControllerHandleAllocator class are not installed correctly, which prevents the creation of new controller handle plugins in packages other than the main moveit_ros_control_interface package (for example, if someone has created a custom controller and want to execute trajectories with it using the TrajectoryExecutionManager). This PR fixes that.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #789 (88fc078) into main (65fdd50) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #789   +/-   ##
=======================================
  Coverage   55.58%   55.58%           
=======================================
  Files         198      198           
  Lines       21420    21420           
=======================================
  Hits        11904    11904           
  Misses       9516     9516           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65fdd50...88fc078. Read the comment docs.

Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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

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.

@schornakj schornakj force-pushed the pr-fix-controller-handle-header-install branch from 32214d8 to fd22eab Compare November 10, 2021 21:41
)

## 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.

@JafarAbdi JafarAbdi merged commit b426a1c into moveit:main Nov 15, 2021
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.

5 participants