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 travis: suppress catkin_lint error for eigenpy #1734

Closed
wants to merge 1 commit into from

Conversation

rhaschke
Copy link
Contributor

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Could you add a TODO to remove this for noetic?

@rhaschke
Copy link
Contributor Author

This is not related to Noetic. We can remove it if catkin_lint relaxes the check.
If @roehling decides not to relax the check, we need to find_package(eigenpy) on its own and consider the corresponding _INCLUDE_DIRS and _LIBRARIES variables explicitly. Currently, they just become part of the corresponding catkin_* variables.
I primarily filed this to fix our Travis, particularly in view of WMD next week.

@v4hn
Copy link
Contributor

v4hn commented Nov 15, 2019

we need to find_package(eigenpy) on its own and consider the corresponding _INCLUDE_DIRS and _LIBRARIES variables explicitly

And that's the way I would like to handle this in the future, as I agree that this should not be a catkin component if it's not built through catkin. Thus the request for the TODO.

Anyway, as we plan to remove the dependency in the future in favor of boost_python anyway, I'm also fine with this as-is until then.

Can be merged as soon as CI succeeds.

@rhaschke
Copy link
Contributor Author

If you agree that eigenpy shouldn't be a catkin component if it's not building via catkin, we should directly prefer a cleaner (but more laborious) solution as suggested in #1734 (comment).
IMHO, if a package provides a package.xml it should be accepted as a proper ROS package that can be imported via find_package(catkin ...). Let's wait for feedback from @roehling.

@v4hn
Copy link
Contributor

v4hn commented Nov 18, 2019

if a package provides a package.xml it should be accepted as a proper ROS package that can be imported via find_package(catkin ...).

I did not look at the actual implementation of the catkin-compatible cmake config.
If it is actually merged (and released) in upstream sources, I would have been fine with your solution too.
The use-case I have in mind is other linux distributions which would package eigenpy as a ROS-agnostic library from upstream sources.

sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Unless a header lives in the same or a child directory of the file
including it, it's recommended to use <> for the #include statement.

For more information, see the C++ Core Guidelines item SF.12

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of-include-for-files-relative-to-the-including-file-and-the-angle-bracket-form-everywhere-else
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

2 participants