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

Conversation

@YuehChuan
Copy link
Contributor

commented Mar 13, 2017

Description

#469
in folder ~/moveit/moveit_kinematics/CMakeLists.txt
comment out ${EIGEN3_INCLUDE_DIRS}

catkin_package(
  LIBRARIES
  INCLUDE_DIRS
#    ${EIGEN3_INCLUDE_DIRS}

https://github.com/YuehChuan/moveit/blob/indigo-devel/moveit_kinematics/CMakeLists.txt#L35

in folder ~/moveit/moveit_kinematics/package.xml ,add cmake modules

<build_depend>cmake_modules</build_depend>
<run_depend>cmake_modules</run_depend>

https://github.com/YuehChuan/moveit/blob/indigo-devel/moveit_kinematics/package.xml#L26

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

Thank you!

@@ -32,12 +32,13 @@ set(THIS_PACKAGE_INCLUDE_DIRS
catkin_package(
LIBRARIES
INCLUDE_DIRS
${EIGEN3_INCLUDE_DIRS}
# ${EIGEN3_INCLUDE_DIRS}

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Mar 13, 2017

Contributor

Could you remove the line instead of commenting it out?

@@ -1,6 +1,6 @@
cmake_minimum_required(VERSION 2.8.3)
project(moveit_kinematics)
find_package(catkin REQUIRED)
find_package(catkin REQUIRED cmake_modules)

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Mar 13, 2017

Contributor

Why is this needed? I seem to recall that using cmake_modules for Eigen is deprecated these days. Besides that I don't see other changes to match with this. If this was needed in order to compile on your system there may be more wrong.

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 14, 2017

Author Contributor

Uhha, you are right. It's for compiling eigen3, and it is deprecated. I'm still using the old style.
Already remove, thanks for let me know this.

@@ -0,0 +1,10 @@
set(MOVEIT_LIB_NAME moveit_kdl_kinematics_plugin)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Mar 18, 2017

Member

none of these KDL files should be in this pull request- it a mistake right?

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 19, 2017

Author Contributor

hey @davetcoleman Dave thanks for reviewing! yep you are right, there is noting to do with KDL plugin. I forgot why i added it. Tones of moveIt stuff I don't know. Is kdl also deprecated or is self-contain in newest version? maybe be the habit from gazebo.

We are try moveIt! on a new collaborative arm robot techman robot made from taiwan
http://tm-robot.com/
Now just made rviz could connect to real robot and gazebo model; And there is some challenge in motion plannning like RRTconnectkConfig; some poses the robot could not reach or the plannig path it's strang. Is there some reference for add constrain or specified the motion?
here are repo's
my lab: https://github.com/ISCI-NCTU/isci_ros_tm5
official: https://github.com/kentsai0319/techman_robot
Just in case you guys get interest and have free time to play, hehehe.
Anyway, this is not relate to this PR. Thanks for review! 😄

moveit_configuration

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

ahuh i remember http://wiki.ros.org/descartes help? 🍒

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Mar 22, 2017

Member

@YuehChuan please keep discussion to original topic in this thread, thank you!

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

ok, thanks for noticing

${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.

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.

Copy link
Contributor Author

left a comment

habonbon

@@ -0,0 +1,10 @@
set(MOVEIT_LIB_NAME moveit_kdl_kinematics_plugin)

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

ahuh i remember http://wiki.ros.org/descartes help? 🍒

${THIS_PACKAGE_INCLUDE_DIRS}
CATKIN_DEPENDS
pluginlib
moveit_core
moveit_ros_planning
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!

@@ -0,0 +1,10 @@
set(MOVEIT_LIB_NAME moveit_kdl_kinematics_plugin)

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

ok, thanks for noticing

${THIS_PACKAGE_INCLUDE_DIRS}
CATKIN_DEPENDS
pluginlib
moveit_core
moveit_ros_planning
DEPENDS Eigen3

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_PACKAGE_INCLUDE_DIRS}
CATKIN_DEPENDS
pluginlib
moveit_core
moveit_ros_planning
DEPENDS Eigen3

This comment has been minimized.

Copy link
@YuehChuan

YuehChuan Mar 31, 2017

Author Contributor

what's ping mean

@IanTheEngineer IanTheEngineer changed the title fix indigo moveit_kinematics Egen3 dependency fix indigo moveit_kinematics Eigen3 dependency Apr 17, 2017
@de-vri-es de-vri-es dismissed their stale review Apr 19, 2017

no longer relevant

Copy link
Contributor

left a comment

The change looks good now, but the whole issue does raise another question:

Do we want to keep using pkg-config for Eigen, or depend on Eigen3Config.cmake (which sadly exports EIGEN3_* variables, though that does mean it is compatible with the current cmake code of moveit).

I'm fine with sticking with pkg-config. I don't really have a strong opinion on the subject.

@v4hn
v4hn approved these changes Apr 20, 2017
@v4hn v4hn dismissed davetcoleman’s stale review Apr 20, 2017

feedback has been addressed

@v4hn

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

Do we want to keep using pkg-config for Eigen, or depend on Eigen3Config.cmake (which sadly exports EIGEN3_* variables, though that does mean it is compatible with the current cmake code of moveit).

We use both ways in MoveIt at the moment.
pkg-config is more backward compatible because until recently upstream eigen3 didn't provide Eigen3Config.cmake in a release.
Other than that it doesn't really matter.

@v4hn v4hn merged commit cf853d5 into ros-planning:indigo-devel Apr 20, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v4hn added a commit that referenced this pull request Apr 20, 2017
fix moveit_kinematics Eigen3 dependency in catkin_package
v4hn added a commit that referenced this pull request Apr 20, 2017
fix moveit_kinematics Eigen3 dependency in catkin_package
@v4hn

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

cherry-picked to j/k

Thank you for the contribution @YuehChuan !

@YuehChuan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

@v4hn Thanks for those feed!

@YuehChuan YuehChuan deleted the YuehChuan:indigo-devel branch Apr 21, 2017
jrgnicho added a commit to jrgnicho/moveit that referenced this pull request May 15, 2017
fix moveit_kinematics Eigen3 dependency in catkin_package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.