search

moveit / moveit_core

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
30 stars 76 forks source link

[reopen] Upgrade to Eigen3 as required in Jade #293

Closed 130s closed 8 years ago

130s commented 8 years ago

Replacing #290

@davetcoleman I confirmed that this works. Tried to open PR against your branch but it seems out of date so I might as well open a new one here.

130s commented 8 years ago

Ping @davetcoleman or anyone who's interested. Thanks!

rhaschke commented 8 years ago

@130s Is https://goo.gl/hNNKD1 the recommended way to fix issues with finding Eigen3 on OSX? From my understanding this issue is not yet resolved, is it?

130s commented 8 years ago

@rhaschke @davetcoleman yeah I'm not sure whether this PR that got a hint from https://goo.gl/hNNKD1 that uses pkg_config is the right solution, particularly on ROS buildfarm where I know some people raised issues using pkg_config. I just confirmed that the build passes on Travis. Sorry but I don't have other idea to move this on. I asked a question but got no answer yet.

rhaschke commented 8 years ago

I looked into that issue a little bit deeper: One issue is that CMAKE_MODULE_PATH is not set. (Should it be set by catkin tools?). But on Trusty (used by Travis CI), there is only a FindEigen3.cmake (in /usr/share/cmake-2.8/Modules) requiring the CMAKE_MODULE_PATH to point there. Eventually, on Xenial this looks different.

On the other hand, finding Eigen3 via pkg-config does work. The fallback to cmake_modules suggested in the reference, is not needed anymore then (actually the latter finds Eigen via pkg-config as well.)

However, case matters! The line should be changed to pkg_search_module(EIGEN3 REQUIRED eigen3) filling the variable EIGEN3_INCLUDE_DIRS - in contrast to EIGEN_INCLUDE_DIR filled by the old FindEigen.cmake of cmake_modules.

Anyway, in the moveit_core package, Eigen3 doesn't need to be pulled in explicitly, because it catkin-depends on eigen_stl_containers and eigen_conversions, which pulls in Eigen as well. That's why it worked, no matter which variables were set or not.

I rebased @130s' branch to jade-devel as branch jade-devel-eigen3. Feel free to continue there. I guess we should squash before merging ;-)

130s commented 8 years ago

@rhaschke thanks for the deeper look. I cherry-picked your commits into this PR, and squashed a little bit (I still left 3 commits to show all the committers who contributed).

rhaschke commented 8 years ago

LGTM.

rhaschke commented 8 years ago

@130s Can you adapt the other MoveIt repos in a similar fashion?