Switch to Eigen/CMake's FindEigen3. #2086
Conversation
jkohnert
force-pushed
the
eigen3_5_0_0
branch
from
53a357d to
6c010ab
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2086 +/- ##
==========================================
- Coverage 41.56% 41.54% -0.02%
==========================================
Files 453 453
Lines 113993 114057 +64
Branches 22543 22547 +4
==========================================
+ Hits 47376 47380 +4
- Misses 66617 66677 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@GillesDuvert thanks. I'm still strggling with the Windows build error, tough. I currently don't have a Windows maschine at hand, so it is a bit hard to debug that stuff... |
|
The Windows test error doesn't really make sense to me: I had this error in an earlier stage and it went away when re-adding the CXX_FLAG. This optimization flag seems unrelated to the error. My next change to exclude Eigen3 v5.0.0 re-introcuded the error alhough only the new version of Eigen3 now wil report an error. Why is this |
|
@jkohnert DLM_REGISTER is called when GDL starts, indeed nothing directly related. |
|
@jkohnert I see you have changed CMAKE_CXX_STANDARD from 11 to 14 , obviously for the best. (and Eigen:: probably makes the best of C++14) |
I am maintaining GDL on some laptops and HPC computers with "old" systems (eg Debian 10, Ubuntu 20.04 LTS ) where I have only old cmake versions (eg 3.13.4 for Debian 10) and it is now working fine for GDL. Please don't broke that !
I am not an expert but I suspect we may have side effects, and also maybe some changes to be made in the code. When I compile GDL on OSX I always received a long list of warnings ... Who has time & competence to clarify that ? Question : is it difficult to have 2 cmake files, not changing the actual one which is very stable for me, and a new one preparing the move directly into Cmake V 4 ? (new syntax, I assume simpler !) My feeling it would be great to have a one or two days face to face meeting fully dedicated to exchange and fix code and discuss/prepare a roadmap, we made it ten years ago in Meudon (Paris Observatory) and it was a great time ! |
|
@GillesDuvert and @alaingdl I'm using Arch, and they switched to Eigen3 v5.0.0 recently. This change breaks the CMake-script currently shipped with GDL. So I made the attempt to fix it. And since Eigen ships CMake-Configurations itself, the first attempt was to get rid of this own implementation and just use what upstream provides so we don't reinvent the wheel. This changed configuration than still works with Eigen3 v5.0.0. But: Eigen3 v5.0.0 breaks compilation when using C++11, since it depends on C++14 (see: https://gitlab.com/libeigen/eigen/-/releases/5.0.0#breaking-changes). At first I tried to exclude Eigen3 v5.0.0 but than I tried to just adjust the C++ standard used and see if it works. Which it does. And since C++14 is more like an "augmentation" to C++11 and AFAIK doesn't break things that are working in C++11, that change should probably be fine. I saw a bunch of additional warnings, though, however: moving forward in the standard seems to be desirable, anyway. 😄 |
|
@alaingdl Debian 10 reached end-of-life more than a year ago, even for LTS suport, see: https://endoflife.date/debian I don't think it's desiable to keep the code it is just to support operation system versions that are declared unsupported upstream. |
|
I'm very much in favor of keeping the head (!) above the water, so using new standards when they are needed. |
|
@GillesDuvert I compiled the code including this patch using my current Arch system. Compilation breaks due to Eigen3 v5.0.0 not supporting C++11 when using C++11 in CMakeLists.txt; compilation works fine when setting C++14 in the CMakeLists.txt. Also all tests run fine when using C++14 in CMakeLists.txt. I have to admint, though, that the actual standard used is currently invisible in the pipline's build logs. So I can't tell for sure whether C++14 was actually used in the Ununtu-pipeline-build. And, of course, I know "it's working on my maschine" isn't a valid argument for merging this PR. AFAIR it is possible to get more build output to be able to check what compile command are used exacly, so I can check that. It is worth noting that all changes work using currently defined CMake-versions, vo v4.X is needed. Even Eigen3's CMake implemenation only requires CMake v3.0. We could also use the existing own FindEigen3-implementation for v1.1.X and simply return a better error message for Eigen3 v5.0.0, which obviously isn't "too old". 😄 This would probably require a different PR. Or we could probably split this PR for upcoming v1.1 versions: use Eigen3's FindEigen3 implementaion and only exclude v5.0.0, so we can stick on C++11 for v1.1. That would only need to revert the latest commit to this PR and make another PR with that last commit. Best, Jan |
|
Setting verbosity for makefile in cmake confirms the code is build in C++14 standard when the setting is active in CMakeLists.txt, see https://github.com/gnudatalanguage/gdl/actions/runs/18036500131/job/51324556556?pr=2086#step:4:600 This again resembles the local build I get. Locally, I currently get two warnings: This is obviously unrelated to the standard change, and just a hint. So IMHO using C++14 seems pretty straight forward. More problems are likely to occur when using C++17 or newer standard versions, since there are breaking changes w.r.t. C++11 I'll revert the change making the makefile verbose, now. Best, Jan. |
This reverts commit 2daa8c5.
Attempt to switch to Eigen3's CMake-implementation. This also supports Eigen3's current release version 5.0.0 as long as the used API still works.