refactor cmake scripts into smaller files #557
Conversation
cmake/handle_eigen.cmake
Outdated
| ### These patches only affect usage of MKL. If you want to enable MKL, you *must* | ||
| ### use our patched version of Eigen | ||
| ### See: http://eigen.tuxfamily.org/bz/show_bug.cgi?id=704 (Householder QR MKL selection) | ||
| ### http://eigen.tuxfamily.org/bz/show_bug.cgi?id=705 (Fix MKL LLT return code) |
There was a problem hiding this comment.
These two are fixed in 2015, I am curious that are these still relevant for us?
There was a problem hiding this comment.
You're right. I checked it and apparently the current embedded copy of Eigen doesn't have any patch applied.
I will remove the comments.
There were a number of comments that looked doubtful to me. Those 100% immediately-clear wrong or outdated were removed, but I left most of them untouched in the first commit of this PR to first check if all tests passed.
|
An additional change: the system version of Eigen3 is used by default if it is successfully first found in the system. In other words:
In any case, one can override the selection with the same good-old cmake variable I hope you agree with this policy. |
I think this is not a bad policy, but it is a change that could break - silently and in a way that is hard to debug - several installs of GTSAM out in the world. Regardless, I think that change should not be part of this PR. If we do it, it should be it’s own PR and widely disseminated/documented. |
Reverted too. |
|
On Wed, Oct 7, 2020 at 5:06 PM Frank Dellaert ***@***.***> wrote:
@dellaert approved this pull request.
Ok, thanks.
This one can be merged now...
|
|
Thanks again !!!!! |
I thought this is a good step before addressing #478 , since adding support for more system libraries will add a lot of extra cmake code, so it might be better to keep it hierarchically organized, one script for each external dependency.
It passes all tests in a local build.