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 indigo moveit_kinematics Eigen3 dependency #470

Merged
merged 5 commits into from Apr 20, 2017
Merged
Changes from 4 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -32,12 +32,12 @@ set(THIS_PACKAGE_INCLUDE_DIRS
catkin_package(
LIBRARIES
INCLUDE_DIRS
${EIGEN3_INCLUDE_DIRS}
${THIS_PACKAGE_INCLUDE_DIRS}
CATKIN_DEPENDS
pluginlib
moveit_core
moveit_ros_planning
DEPENDS Eigen3

This comment has been minimized.

Copy link
@v4hn

v4hn Mar 20, 2017

Member

This doesn't work.
pkg_search_module(EIGEN3 REQUIRED eigen3) defines a variable EIGEN3_INCLUDE_DIRS, but
the DEPENDS Eigen3 makes catkin look for Eigen3_INCLUDE_DIRS.

Please change this to (including the newline+indention)

DEPENDS
  EIGEN3

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

@v4hn thanks for review!
But , I didn't get it.
According to http://answers.ros.org/question/58498/what-is-the-purpose-of-catkin_depends/
boost directly put after DEPENDS Boost. And seems .travis build check ready pass. I would like to take your suggestion, but curious is this just typesetting issue but not relate to compiling process? Thanks!

This comment has been minimized.

Copy link
@130s

130s Mar 30, 2017

Member

Ping @YuehChuan :)

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

@130s 64 bytes from YuehCHuan: icmp_seq=31 ttl=64 time=0.189 ms

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

what's ping mean

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

OH i didn't submit mine review 10 days ago seems they were pending, got it, thanks @130s

This comment has been minimized.

This comment has been minimized.

Copy link
@v4hn

v4hn Mar 31, 2017

Member

Passing on Eigen dependencies in ROS is a bit of a curiosity really. It was/(is?) broken in many places and nobody ever notices, because every dependent project still includes it somehow either directly or through different dependency.
It doesn't surprise me that travis succeeds even when your fix probably does not work.
This is only made worse by the fact that until a couple of months ago only Debian's version of eigen3 provided the variable EIGEN3_INCLUDE_DIRS (notice the trailing S) and everyone else only set EIGEN3_INCLUDE_DIR (no S). Anyway..

But , I didn't get it.

Variables in cmake are case-sensitive.
catkin_package's DEPENDS <package> field invokes some logic that automatically adds the contents of the variables <package>_INCLUDE_DIRS and <package>_LIBRARIES to projects that find_package this catkin package. See here.

So with the code you currently propose catkin_package will look at the variables Eigen3_INCLUDE_DIRS and Eigen3_LIBRARIES.
eigen is header-only, so the latter doesn't matter.
But the variable Eigen3_INCLUDE_DIRS doesn't exist either. It should be looking for EIGEN3_INCLUDE_DIRS instead.

The newline is just a matter of taste and imho fits better with the rest of the CMakeLists.txt.

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Apr 16, 2017

Author Contributor

@v4hn thanks, you are so kind:)
Great insite for explaning eigen so detail.
I'm sorry for reply this late. Your advices really help me learn a lot.
Most people write CmakeLists.txt using template without understanding
what has been happened inside actually. Yes, add Eigen dependencies is out of curiosity.
To tell the truth I may not quite understand the meaning before you give the explanation.

Anyway thank you. Ping me more often please, great teacher. I'll reply it soon next time.:p
(Mailbox full of github notifications...)

This comment has been minimized.

Copy link
@v4hn

v4hn Apr 19, 2017

Member

Could you please address my feedback then? :)

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Apr 19, 2017

Author Contributor

@v4hn Thank you! Changed.

)

include_directories(${THIS_PACKAGE_INCLUDE_DIRS}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.