Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

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

Merged
merged 3 commits into from
May 18, 2016

Conversation

130s
Copy link
Contributor

@130s 130s commented May 9, 2016

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
Copy link
Contributor Author

130s commented May 12, 2016

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

@rhaschke
Copy link
Contributor

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

# Eigen3 hack for Jade http://wiki.ros.org/jade/Migration#Eigen_CMake_Module_in_cmake_modules
find_package(Eigen3)
find_package(PkgConfig)
pkg_search_module(Eigen3 REQUIRED eigen3)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this hack is the same as the one in the link you reference, they look pretty different to me.

@130s
Copy link
Contributor Author

130s commented May 17, 2016

@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
Copy link
Contributor

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 130s force-pushed the davetcoleman/jade-devel-eigen branch from 9ea7d1f to 56a56d0 Compare May 17, 2016 23:58
@130s 130s force-pushed the davetcoleman/jade-devel-eigen branch from 56a56d0 to 69e12f9 Compare May 18, 2016 00:10
@130s
Copy link
Contributor Author

130s commented May 18, 2016

@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
Copy link
Contributor

LGTM.

@rhaschke rhaschke merged commit 96a646d into moveit:jade-devel May 18, 2016
@rhaschke
Copy link
Contributor

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

@130s 130s deleted the davetcoleman/jade-devel-eigen branch May 18, 2016 17:34
@130s 130s mentioned this pull request Jul 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants