search

ICRAR / leap-accelerate

Low-frequency Excision of the Atmosphere in Parallel
GNU General Public License v2.0
1 stars 1 forks source link

Fix eigen includes #30

Closed rtobar closed 4 years ago

rtobar commented 4 years ago

This is the issue I mentioned in one of my comments recently and I thought at least I'd open an issue so we don't forget about this.

The way Eigen headers are being included seems to be incorrect, as they are being included with the eigen3 prefix (e.g., #include <eigen3/Eigen/Core) rather than directly (e.g., #include <Eigen/Core>). The latter form is how includes appear in the Eigen examples. And even without considering that, the former building leap-accelerate against arbitrary Eigen installations not possible.

I believe the correct way of doing this is to remove all eigen3 prefixes from all includes, and to link in cmake against the Eigen3::Eigen target, which will include the eigen_installation/include/eigen3 directory in its include directory search path. There was a comment on the CMakeLists.txt file about this not working in xenial, but since a known, very specific Eigen version is being used this shouldn't be a problem.

I'm happy to provide a patch, but would be rather invasive as it will touch all the eigen includes. Maybe it's better to wait until things have stabilized a bit.

calgray commented 4 years ago

This is something I'd been meaning to do after finding the current Eigen version and it sounds like CMake should be able find the Eigen3::Eigen target now on all builds. I haven't tried building and installing the Eigen headers on the cluster machine yet so there may be some extra setup to deploy there.

Feel free to create a pull request that removes the fix header and updates the eigen paths.