Use eigen3 config.cmake #1304

OznOg · 2022-10-07T16:56:38Z

This fixes bug #1297

varunagrawal · 2022-10-07T21:13:04Z

cmake/HandleEigen.cmake

-    set(GTSAM_EIGEN_INCLUDE_FOR_INSTALL "${EIGEN3_INCLUDE_DIR}")
+    # The actual include directory (for BUILD cmake target interface):
+    # Node Eigen cmake is completelly broken on some version,
+    # EIGEN3_INCLUDE_DIR is just unusable as it points to some random location.


Does this mean there is no guarantee this will work everywhere and for everyone?

This means it is really ugly, and should not be necessary on some versions. This works since eigen 3.3.0. Prior Eigen will not, but is not supported anyway AFAIK.

Then we should probably replace the text with a comment stating that it will work for Eigen 3.3.0 onwards but may not for prior versions. Basically what you said in your comment. 🙂

This is also in the commit message, but I'll update the comment.

Since Eigen 3.3.0, a Config.cmake file is provided, thus no need to rely on a custom one. Moreover, the FindEigen3.cmake used in gtsam was erroneously forcing an include directory when using system version of eigen. This fixes bug borglab#1297

varunagrawal · 2022-10-08T17:34:12Z

@OznOg this PR is great. I just need to test it out with the Matlab wrapper (we don't have a CI for it) and if that works, I'll approve and merge. :)

OznOg · 2022-10-08T18:08:02Z

@OznOg this PR is great. I just need to test it out with the Matlab wrapper (we don't have a CI for it) and if that works, I'll approve and merge. :)

Ok, good news

OznOg · 2022-10-21T16:17:25Z

Did you have time to test with matlab? any issue?

varunagrawal · 2022-10-22T23:57:54Z

@OznOg I've been having issues with my Matlab machine which should be resolved by this weekend. Apologies for the delay, this is my fault.

dellaert · 2022-10-23T18:43:02Z

Letting @varunagrawal continue the review, but one question I had: there are two Eigen versions people can use: the one included with GTSAM, and system. Will this PR work with both? I know both options are being used. Not sure whether our CI is good at testing both :-/

varunagrawal · 2022-10-24T23:08:42Z

Letting @varunagrawal continue the review, but one question I had: there are two Eigen versions people can use: the one included with GTSAM, and system. Will this PR work with both? I know both options are being used. Not sure whether our CI is good at testing both :-/

@dellaert yes the CI tests for System Eigen as well. It's in build-special.yml under the ubuntu-system-libs workflow. 🙂

varunagrawal · 2022-10-25T14:19:49Z

Matlab tests pass! Approving.

varunagrawal

LGTM

dellaert · 2022-10-25T19:04:44Z

Uhm, now can't compile anymore:

In file included from /Users/dellaert/git/github/gtsam/base/Matrix.h:28:
/Users/dellaert/git/github/gtsam/base/Vector.h:74:1: error: static_assert failed due to requirement '3 == 3 && 3 == 4' "Error: GTSAM was built against a different version of Eigen"
static_assert(

What's the fix?

jlblancoc · 2022-10-25T19:34:23Z

Hi Frank! Was it building the gtsam lib itself, or an example/app?
Have you tried checking what's the directory shown by cd BuildDir && grep Eigen CMakeCache.txt?

(Note: I've not tested the merged version yet!)

jlblancoc · 2022-10-25T19:43:13Z

@OznOg @varunagrawal : Is there any reason to leave a copy of Eigen3 under gtsam/3rdparty? I think it's useless "as is" right now, because it must be processed by cmake first to produce a usable EigenConfig.cmake file... Perhaps it could be removed and installation instructions updated to remember Eigen is a mandatory prerequisite, just like Boost?

varunagrawal · 2022-10-25T19:44:06Z

Seems like CMake is picking up a different version of Eigen (3.4 rather than the provided 3.3). Are you on a M1 Mac?

varunagrawal · 2022-10-25T19:51:03Z

@OznOg @varunagrawal : Is there any reason to leave a copy of Eigen3 under gtsam/3rdparty? I think it's useless "as is" right now, because it must be processed by cmake first to produce a usable EigenConfig.cmake file... Perhaps it could be removed and installation instructions updated to remember Eigen is a mandatory prerequisite, just like Boost?

I believe the idea is that by including Eigen under 3rdparty, we can compile out of the box and have some control over the Eigen version. @dellaert prefers this approach since it requires no wasted effort in integration.

varunagrawal · 2022-10-25T19:55:03Z

@dellaert FWIW I compiled (and cleaned and compiled) the latest develop on my M1 mac and I don't seem to have this problem. You may want to do a full rebuild?

varunagrawal · 2022-10-25T19:59:27Z

I think I found the issue. Line 5 in HandleEigen.cmake sets USE_SYSTEM_EIGEN_INITIAL_VALUE to ON which tells Cmake to use the system Eigen. Looks like @OznOg's PR unearthed a bug in the cmake. 🙂

@dellaert you should be able to compile by setting the GTSAM_USE_SYSTEM_EIGEN flag to false.

dellaert · 2022-10-26T01:24:50Z

I think I found the issue. Line 5 in HandleEigen.cmake sets USE_SYSTEM_EIGEN_INITIAL_VALUE to ON which tells Cmake to use the system Eigen. Looks like @OznOg's PR unearthed a bug in the cmake. 🙂

@dellaert you should be able to compile by setting the GTSAM_USE_SYSTEM_EIGEN flag to false.

That does not work, but #1314 does fix it for me. I'll approve and merge that one...

OznOg · 2022-10-26T18:44:33Z

Hi guys, is there something I can/need to do to help, or is everything ok now?

varunagrawal · 2022-10-26T18:59:46Z

Everything has been figured out. Thanks for following up. :)

Use cannonical library name for eigen

ec3d6b3

ProfFan requested review from jlblancoc, varunagrawal and ProfFan October 7, 2022 20:48

varunagrawal reviewed Oct 7, 2022

View reviewed changes

OznOg requested review from varunagrawal and removed request for ProfFan, jlblancoc and varunagrawal October 8, 2022 09:44

Use Eigen Config file for cmake detection

774cfd2

Since Eigen 3.3.0, a Config.cmake file is provided, thus no need to rely on a custom one. Moreover, the FindEigen3.cmake used in gtsam was erroneously forcing an include directory when using system version of eigen. This fixes bug borglab#1297

OznOg force-pushed the UseEigen3Config.cmake branch from bbb7d93 to 774cfd2 Compare October 8, 2022 16:05

ProfFan linked an issue Oct 9, 2022 that may be closed by this pull request

gtsam should use eigen cmake config file #1297

Closed

varunagrawal approved these changes Oct 25, 2022

View reviewed changes

varunagrawal merged commit 3d4236e into borglab:develop Oct 25, 2022

varunagrawal mentioned this pull request Oct 25, 2022

Fix CMake so that Eigen discovery and linking is consistent #1314

Merged

OznOg deleted the UseEigen3Config.cmake branch October 26, 2022 18:44

FedericoRollo mentioned this pull request Jan 25, 2024

GTSAM was built against a different version of Eigen #1717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use eigen3 config.cmake #1304

Use eigen3 config.cmake #1304

OznOg commented Oct 7, 2022

varunagrawal Oct 7, 2022

OznOg Oct 8, 2022

varunagrawal Oct 8, 2022

OznOg Oct 8, 2022

OznOg Oct 8, 2022

varunagrawal commented Oct 8, 2022

OznOg commented Oct 8, 2022

OznOg commented Oct 21, 2022

varunagrawal commented Oct 22, 2022

dellaert commented Oct 23, 2022

varunagrawal commented Oct 24, 2022

varunagrawal commented Oct 25, 2022

varunagrawal left a comment

dellaert commented Oct 25, 2022

jlblancoc commented Oct 25, 2022

jlblancoc commented Oct 25, 2022

varunagrawal commented Oct 25, 2022 •

edited

Loading

varunagrawal commented Oct 25, 2022

varunagrawal commented Oct 25, 2022

varunagrawal commented Oct 25, 2022

dellaert commented Oct 26, 2022

OznOg commented Oct 26, 2022

varunagrawal commented Oct 26, 2022

Use eigen3 config.cmake #1304

Use eigen3 config.cmake #1304

Conversation

OznOg commented Oct 7, 2022

varunagrawal Oct 7, 2022

Choose a reason for hiding this comment

OznOg Oct 8, 2022

Choose a reason for hiding this comment

varunagrawal Oct 8, 2022

Choose a reason for hiding this comment

OznOg Oct 8, 2022

Choose a reason for hiding this comment

OznOg Oct 8, 2022

Choose a reason for hiding this comment

varunagrawal commented Oct 8, 2022

OznOg commented Oct 8, 2022

OznOg commented Oct 21, 2022

varunagrawal commented Oct 22, 2022

dellaert commented Oct 23, 2022

varunagrawal commented Oct 24, 2022

varunagrawal commented Oct 25, 2022

varunagrawal left a comment

Choose a reason for hiding this comment

dellaert commented Oct 25, 2022

jlblancoc commented Oct 25, 2022

jlblancoc commented Oct 25, 2022

varunagrawal commented Oct 25, 2022 • edited Loading

varunagrawal commented Oct 25, 2022

varunagrawal commented Oct 25, 2022

varunagrawal commented Oct 25, 2022

dellaert commented Oct 26, 2022

OznOg commented Oct 26, 2022

varunagrawal commented Oct 26, 2022

varunagrawal commented Oct 25, 2022 •

edited

Loading