-
Notifications
You must be signed in to change notification settings - Fork 29
Fix compatibility with Eigen3 5.0.0 and bump version to 0.10.1 #105
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
Conversation
@GiulioRomualdi as I guess you are more in contact with @xEnVrE do you think he is interested in checking this out, or we should merge and do a new release on our own? |
Hi @traversaro If I got it correctly, this will just fix usage of the library for users adopting the newer Eigen. Should we have older projects, e.g. running in dockerized envs with older versions of Eigen, this will have no impact on them (both in terms of CMake and SVD usage). Is it right? Thanks |
Happy to see you active. :D Yes, that is correct. Users of older Eigen will just start linking to the At the C++ level, the preprocessor logic ensures that users of older Eigen still use the old code. |
Done in 4dc452e . |
:D
Fine with me then. Thanks for the clarification! Not sure whether CI is still running. I guess it would be good to let the tests run. Thanks |
Thanks @xEnVrE ! Just to understand, I guess then it is ok if I merge? |
@traversaro if you feel the CI checks (which btw seems stuck) are not necessary, fine with me to proceed! |
On the CMake side, there has always been a typo in the logic to link to Eigen3, but everything was working as
EIGEN3_INCLUDE_DIR
was a valid variable. As with Eigen3 5.0.0 is not exported anymore, fixing the typo is necessary to ensure that the library builds with Eigen3 5.0.0.On the c++ side, there was a breaking change in how SVD is invoked, see https://gitlab.com/libeigen/eigen/-/merge_requests/826/ for more details.
xref: robotology/robotology-superbuild#1902
xref: robotology/robotology-superbuild#1903
xref: conda-forge/eigen-feedstock#47