-
Notifications
You must be signed in to change notification settings - Fork 51
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
adding a FindEigen.cmake module #11
Conversation
|
||
find_package(PkgConfig) | ||
pkg_check_modules(PC_EIGEN eigen3) | ||
set(EIGEN_DEFINITIONS ${PC_EIGEN_CFLAGS_OTHER}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we getting the include dirs from the pc file as well?. In my installation (libeigen3-dev 3.0.5-1), /usr/share/pkgconfig/eigen3.pc
looks like this:
Name: Eigen3
Description: A C++ template library for linear algebra: vectors, matrices, and related algorithms
Requires:
Version: 3.0.5
Libs:
Cflags: -I/usr/include/eigen3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was copied over from catkin, I'll look at improving this to use pc for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that the pc file doesn't work correctly on Windows? That is the only reason to search instead, but that seems unlikely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh the other problem is that pkg-config only gives you the cflags, I would have to split and extract the include paths so that CMake can use them. I'm not sure which is better, but since the current implementation is in use already I would lean towards it.
I'd like to bring your attention to the fact that there currently exist multiple scripts for finding Eigen, each of which has its own set of variables and capitalization policy. On the one hand, the Eigen project already provides:
This PR proposes yet another script/ set of variables, but offers consistent naming between what gets passed to |
# Cache variables (not intended to be used in CMakeLists.txt files) | ||
# | ||
# - Eigen_INCLUDE_DIR: Absolute path to package headers. | ||
# - Eigen_LIBRARY: Absolute path to the library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed, as the variable does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this was a copy-paste-edit of the TinyXML documentation.
I basically copied over the |
I am using Eigen 3.2.0 on my machine and I still do not have the I suppose we could use the Thoughts? |
The feature was added post-3.2, so we can't really use it as of now. It's only in the default branch.
I don't see the advantage either, especially because the variable names are significantly different. The script is looking good now. |
@adolfo-rt thanks for the review. |
adding a FindEigen.cmake module
@dirk-thomas @adolfo-rt can I get a review of this if you have time?