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 eigen3 depends #6

Merged
merged 2 commits into from
Sep 28, 2016
Merged

Fix eigen3 depends #6

merged 2 commits into from
Sep 28, 2016

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Sep 26, 2016

In addition to #5, fixed EIGEN3_INCLUDE_DIRS problem, reported at ros/robot_model#149

Xenial uses eigen 3.5, where as Willy uses 3.2 and 3.2 only provides EIGEN3_INCLUDE_DIR where eigen 3.5 provides EIGEN3_INCLUDE_DIRS
otherwise will have following warning and could not build as reporeted at ros/robot_model#149

Warnings   << eigen_stl_containers:check /home/catkin_ws/logs/eigen_stl_containers/build.check.001.log                             
CMake Warning at /opt/ros/kinetic/share/catkin/cmake/catkin_package.cmake:166 (message):
  catkin_package() DEPENDS on 'EIGEN' but neither 'EIGEN_INCLUDE_DIRS' nor
  'EIGEN_LIBRARIES' is defined.
Call Stack (most recent call first):
  /opt/ros/kinetic/share/catkin/cmake/catkin_package.cmake:102 (_catkin_package)
  CMakeLists.txt:7 (catkin_package)

This is the correct way to make use of catkin_package's DEPENDS
flag. It was plain broken before and I'm pretty sure nobody ever
checked.
endif()
if(NOT EIGEN3_LIBRARIES)
set(EIGEN3_LIBRARIES)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to define the libraries variable. catkin only complains if both variables are undefined - if one is defined it won't print a warning (see https://github.com/ros/catkin/blob/54b7beb709d46c2f5cfb747901907901e954d51e/cmake/catkin_package.cmake#L165-L167).

# build flags in catkin_package(), we still have to find Eigen3 here
find_package(Eigen3 REQUIRED)

# eigen 3.2 (willy) only provdies EIGEN3_INCLUDE_DIR, not EIGEN3_INCLUDE_DIRS
Copy link
Member

Choose a reason for hiding this comment

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

@dirk-thomas
Copy link
Member

It would probably be best to run a prerelease job to check that this fixes the problem on Wily before releasing this.

@k-okada
Copy link
Contributor Author

k-okada commented Sep 26, 2016

thanks for a comment, fixed.
if there is something that I need to do to run a prerelase job, please let me know

@dirk-thomas
Copy link
Member

A prerelease needs to be run locally. http://prerelease.ros.org can help to configure the command line options but I guess for this case (with the PR on your fork) some manual fiddling might be necessary.

It would be great if you could run one, otherwise the maintainer of this package might do it before merging / releasing.

@k-okada
Copy link
Contributor Author

k-okada commented Sep 27, 2016

A prerelease needs to be run locally. http://prerelease.ros.org can help to configure the command line options

sorry, I have very little knowledge on new technologies like ajax, django..., so what I can do is just running dpkg-buildpackage -> https://gist.github.com/anonymous/415bb06484c4e6bee755ac1a4b7611ad

@k-okada
Copy link
Contributor Author

k-okada commented Sep 27, 2016

also able to run prelease on local machine -> https://gist.github.com/anonymous/6f5dc9f9ba4255fee33fae92405a000c

@dirk-thomas
Copy link
Member

@k-okada Thank you for working on the patch as well as testing with the prerelease. It looks good to me.

@wjwwood This seems to address ros/robot_model#149.

@v4hn
Copy link
Contributor

v4hn commented Sep 27, 2016

@k-okada please highlight me next time, I found this request only by chance.

+1 The additional patch makes total sense and I actually have no idea why I forgot to handle this case.

Thanks for looking at this @dirk-thomas ! I'm fully confident this does not break anything but allows a number of packages to remove ${EIGEN3_INCLUDE_DIR} from their exported INCLUDE_DIRS.

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

4 participants