[cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. #1281

jnbrunet · 2020-03-23T07:40:02Z

Following the rise of discussions about further using Eigen inside SOFA, this PR aims at standardizing its inclusion in the framework.

Right now, SOFA automatically pulls a fixed version of Eigen (3.2). It also install it along with SOFA, which is a big problem when an external program or library tries to compile with both SOFA and another version of Eigen.

This PR removes the automatic pull of Eigen, and instead rely on the standard find_package(Eigen3) which is available in almost all distributions via the system package manager (apt, dnf, pacman, etc), for instance:

Debian 9	Eigen 3.3.2
Debian 10	Eigen 3.3.7
Ubuntu 18.04	Eigen 3.3.4
Ubuntu 19.10	Eigen 3.3.7
Fedora 29	Eigen 3.3.5
Fedora 30-31	Eigen 3.3.7

Right now, this PR compiles with all of the previous distributions (I've tested it with Docker by simply installing eigen3-devel and compiling Sofa). However, there is a faulty test with Eigen > 3.2: the compliant_test from the Compliant plugin. It seems to come from the Assembly visitor of the plugin, which relies a lot on Eigen to build the system matrices. I'll have a look at it in the next days, but if someone have an idea...
Found the nasty bug at 53dd4f5 - memcopy is dangerous !

Everything ~~else~~ seems to be fine with recent versions of Eigen.

This PR:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings.
does not generate new unit test failures.
does not generate new scene test failures.
does not break API compatibility.
is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

guparan

Thank you @jnbrunet for this very nice work!
My remarks mainly concern OpenMP dependency, as you will see in the review comments.
As a reminder, please have a look at my gist about How to correctly add and propagate a dependency 😉

guparan · 2020-03-23T17:07:07Z

SofaKernel/SofaCommon/SofaCommonConfig.cmake.in

@@ -3,9 +3,19 @@
 @PACKAGE_INIT@

 set(SOFACOMMON_TARGETS @SOFACOMMON_TARGETS@)
+set(SOFAEIGEN2SOLVER_WITH_OPENMP @SOFAEIGEN2SOLVER_WITH_OPENMP@)


Standard naming is SOFAEIGEN2SOLVER_HAVE_OPENMP

guparan · 2020-03-23T17:08:43Z

SofaKernel/SofaCommon/SofaCommonConfig.cmake.in

+find_package(Eigen3 REQUIRED)
+
+# OpenMP might be required by SofaEigen2Solver
+if (SOFAEIGEN2SOLVER_WITH_OPENMP AND NOT TARGET OpenMP::OpenMP_CXX)


Standard naming is SOFAEIGEN2SOLVER_HAVE_OPENMP
Why AND NOT TARGET OpenMP::OpenMP_CXX ?

Because the FindOpenMP.cmake does not check if the target is already defined and starts to look again, which resulted in a very very slow and verbose cmake call (since a lot of modules depends on SofaCommon, and everyone of them calling find_package(OpenMP) because of this)

guparan · 2020-03-23T17:09:39Z

SofaKernel/SofaCommon/config.h.in

@@ -94,5 +94,7 @@
 #  define SOFA_EIGEN2_SOLVER_API SOFA_IMPORT_DYNAMIC_LIBRARY
 #endif

+#cmakedefine SOFAEIGEN2SOLVER_WITH_OPENMP


#cmakedefine01 SOFAEIGEN2SOLVER_HAVE_OPENMP

guparan · 2020-03-23T17:23:30Z

SofaKernel/modules/SofaEigen2Solver/CMakeLists.txt

+if (SOFA_OPENMP AND "${Eigen3_VERSION}" VERSION_LESS 3.2.9)
+    set(SOFAEIGEN2SOLVER_WITH_OPENMP 1 PARENT_SCOPE)
+endif()
+
+if (SOFAEIGEN2SOLVER_WITH_OPENMP)
+    find_package(OpenMP REQUIRED)
+    list(APPEND HEADER_FILES EigenBaseSparseMatrix_MT.h)
+endif()


if(Eigen3_VERSION VERSION_LESS 3.2.9) sofa_find_package(OpenMP BOTH_SCOPES) # will set/update SOFAEIGEN2SOLVER_HAVE_OPENMP else() sofa_set_01(SOFAEIGEN2SOLVER_HAVE_OPENMP VALUE FALSE BOTH_SCOPES) endif() if(SOFAEIGEN2SOLVER_HAVE_OPENMP) list(APPEND HEADER_FILES EigenBaseSparseMatrix_MT.h) endif()

guparan · 2020-03-23T17:26:48Z

SofaKernel/modules/SofaEigen2Solver/CMakeLists.txt

+if (SOFAEIGEN2SOLVER_WITH_OPENMP)
+    target_link_libraries(${PROJECT_NAME} PUBLIC OpenMP::OpenMP_CXX)
+endif()


SOFAEIGEN2SOLVER_HAVE_OPENMP

guparan · 2020-03-23T17:38:34Z

cmake/Modules/FindEigen3.cmake

  find_path(EIGEN3_INCLUDE_DIR NAMES signature_of_eigen3_matrix_library
      HINTS
-      ${EIGEN3_ROOT}
+      ENV EIGEN3_ROOT


This is redundant with the CMake policy CMP0074, which is OLD by default so any <PackageName>_ROOT (env or CMake) is ignored before checking the HINTS and prints a lot of warnings.
I would agree setting this policy to NEW in SOFA. It would also make this hint unnecessary.

guparan · 2020-03-23T17:44:07Z

cmake/Modules/FindEigen3.cmake

+if (NOT TARGET Eigen3::Eigen AND EIGEN3_VERSION_OK AND EIGEN3_INCLUDE_DIR)
+  add_library(Eigen3::Eigen INTERFACE IMPORTED)
+  set_target_properties(Eigen3::Eigen PROPERTIES
+          INTERFACE_INCLUDE_DIRECTORIES "${EIGEN3_INCLUDE_DIR}")
+endif ()


Targets are nice 👍

jnbrunet · 2020-03-24T07:47:57Z

Thanks for your review @guparan !

I've made the changes.

guparan · 2020-03-24T16:20:36Z

applications/plugins/CGALPlugin/FindCGAL.cmake

+  # Build and run test program
+  include(CheckCXXSourceRuns)
+  check_cxx_source_runs("
+// CGAL test program
+
+#include <CGAL/Simple_cartesian.h>
+#include <CGAL/Point_3.h>
+#include <CGAL/Polyhedron_3.h>
+typedef CGAL::Simple_cartesian<double> SCK;
+typedef SCK::Point_3 Point;
+typedef CGAL::Polyhedron_3<SCK> Polyhedron_3;
+
+int main()
+{
+  // CGAL points
+  Point p1(0, 0, 0);
+  Point p2(1, 0, 0);
+  Point p3(0, 1, 0);
+  Point p4(0, 0, 1);
+
+  Polyhedron_3 P;
+  P.make_tetrahedron(p1, p2, p3, p4);
+
+  return 0;
+}
+" CGAL_TEST_RUNS)


This fails on CentOS... Any idea why @jnbrunet ?

Error is

FAILED: CMakeFiles/cmTC_151ec.dir/src.cxx.o /opt/rh/llvm-toolset-7/root/usr/bin/clang++ -Wall -W -DCGAL_TEST_RUNS -o CMakeFiles/cmTC_151ec.dir/src.cxx.o -c src.cxx In file included from src.cxx:4: In file included from /usr/local/include/CGAL/Simple_cartesian.h:29: In file included from /usr/local/include/CGAL/Cartesian/Cartesian_base.h:29: In file included from /usr/local/include/CGAL/basic.h:30: /usr/local/include/CGAL/config.h:122:10: fatal error: 'boost/config.hpp' file not found #include <boost/config.hpp> ^~~~~~~~~~~~~~~~~~ 1 error generated.

After spending too much time on this, I disabled the build test with -DCGAL_TEST_RUNS=TRUE.
Result: CGALPlugin build and tests are OK.

Nevermind, I fixed this issue. No need to -DCGAL_TEST_RUNS=TRUE 👍

use the one installed on the system.

…are of type StorageIndex instead of Index

"All uses of target_link_libraries with a target must be either all-keyword or all-plain."

…en from within Sofa.

jnbrunet · 2020-03-26T21:37:26Z

[ci-build][with-all-tests]

SOFAEIGEN2SOLVER_WITH_OPENMP --> SOFAEIGEN2SOLVER_HAVE_OPENMP

guparan · 2020-03-31T14:01:43Z

SofaKernel/SofaCommon/config.h.in

@@ -94,7 +94,7 @@
 #  define SOFA_EIGEN2_SOLVER_API SOFA_IMPORT_DYNAMIC_LIBRARY
 #endif

-#cmakedefine01 SOFAEIGEN2SOLVER_HAVE_OPENMP
+#cmakedefine SOFAEIGEN2SOLVER_HAVE_OPENMP


If you do this you won't be able to do if(SOFAEIGEN2SOLVER_HAVE_OPENMP) in the code.
Why then?

@guparan Where have you seen if(SOFAEIGEN2SOLVER_HAVE_OPENMP) in the code?

It is only used as a preprocessor token, as it should

#ifdef SOFAEIGEN2SOLVER_HAVE_OPENMP

guparan · 2020-03-31T14:05:40Z

SofaKernel/modules/SofaEigen2Solver/CMakeLists.txt

@@ -23,8 +23,6 @@ find_package(Eigen3 REQUIRED)

 if (SOFA_OPENMP AND "${Eigen3_VERSION}" VERSION_LESS 3.2.9)
    sofa_find_package(OpenMP BOTH_SCOPES) # will set/update SOFAEIGEN2SOLVER_HAVE_OPENMP
-else()
-    sofa_set_01(SOFAEIGEN2SOLVER_HAVE_OPENMP VALUE FALSE BOTH_SCOPES)
 endif()


Without the else, if you decide to disable SOFA_OPENMP after having it enabled, you will still have SOFAEIGEN2SOLVER_HAVE_OPENMP = 1 in CMake cache.

Not if #cmakedefine is used instead of #cmakedefine01

Sorry, I've misread you.

SOFAEIGEN2SOLVER_HAVE_OPENMP isn't a cmake cached variable. It is unset at the beginning of each cmake call. I've tested it:

$ sed -i 's/SOFA_OPENMP:BOOL=ON/SOFA_OPENMP:BOOL=OFF/' CMakeCache.txt $ cmake . $ grep SOFAEIGEN2SOLVER_HAVE_OPENMP include/SofaCommon/config.h /* #undef SOFAEIGEN2SOLVER_HAVE_OPENMP */ $ sed -i 's/SOFA_OPENMP:BOOL=OFF/SOFA_OPENMP:BOOL=ON/' CMakeCache.txt $ cmake . $ grep SOFAEIGEN2SOLVER_HAVE_OPENMP include/SofaCommon/config.h #define SOFAEIGEN2SOLVER_HAVE_OPENMP $ sed -i 's/SOFA_OPENMP:BOOL=ON/SOFA_OPENMP:BOOL=OFF/' CMakeCache.txt $ cmake . $ grep SOFAEIGEN2SOLVER_HAVE_OPENMP include/SofaCommon/config.h /* #undef SOFAEIGEN2SOLVER_HAVE_OPENMP */

Indeed, thanks for the tests 👍
What happens if #cmakedefine01 is used instead?

OFF : #define SOFAEIGEN2SOLVER_HAVE_OPENMP 0
ON : #define SOFAEIGEN2SOLVER_HAVE_OPENMP 1
OFF : #define SOFAEIGEN2SOLVER_HAVE_OPENMP 0

and in cpp files, #ifdef SOFAEIGEN2SOLVER_HAVE_OPENMP will always be true since it is always defined

Yes because the idea behind #cmakedefine01 is to permit the usage of #if and if(). #ifdef surely becomes useless though.

jnbrunet · 2020-03-31T18:58:17Z

[ci-build][with-all-tests]

damienmarchal · 2020-04-01T08:39:23Z

SofaKernel/modules/SofaEigen2Solver/EigenBaseSparseMatrix.h

@@ -311,7 +303,7 @@ class EigenBaseSparseMatrix : public defaulttype::BaseMatrix
    void mult_MT( VectorEigen& result, const VectorEigen& data )
    {
        compress();
-#ifdef _OPENMP
+#ifdef SOFAEIGEN2SOLVER_HAVE_OPENMP


#if (SOFAEIGEN2SOLVER_HAVE_OPENMP == 1)
#...

hugtalbot · 2020-04-01T08:51:07Z

Minor remaining change use pre processor #if condition instead of #ifdef (since it allows to detect more various cases: is defined and true, defined and false or undefined)
CGAL : remove changes and integrate changes from #1308
→ back to wip

…eprocessor tokens

jnbrunet · 2020-04-02T08:15:04Z

[ci-build][with-all-tests]

jnbrunet · 2020-04-02T19:09:31Z

@guparan and @damienmarchal it should be good now, if you want to double check

guparan · 2020-04-03T10:08:52Z

Bye bye embedded Eigen ✋

jnbrunet requested review from matthieu-nesme, guparan and fjourdes March 23, 2020 07:40

jnbrunet added the pr: status wip Development in the pull-request is still in progress label Mar 23, 2020

hugtalbot changed the title ~~Remove the use of an internal Eigen3 version and instead use the one installed on the system.~~ [cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. Mar 23, 2020

guparan suggested changes Mar 23, 2020

View reviewed changes

guparan reviewed Mar 24, 2020

View reviewed changes

guparan added pr: breaking Change possibly inducing a compilation error pr: clean Cleaning the code pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Mar 25, 2020

hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Mar 25, 2020

jnbrunet force-pushed the remove_eigen_from_within_sofa branch from 6d0016d to f94e704 Compare March 26, 2020 07:23

jnbrunet and others added 4 commits March 26, 2020 17:43

Remove the use of an internal Eigen3 version and instead

2ea4fb0

use the one installed on the system.

[Compliant] Fix wrong memcpy for Eigen > 3.2 where the outer indices …

3600ca0

…are of type StorageIndex instead of Index

[SofaSimpleGui] FIX CMake error

e89591f

"All uses of target_link_libraries with a target must be either all-keyword or all-plain."

[cmake] Cleanups of some CMake files relative to the exclusion of Eig…

1e88c3f

…en from within Sofa.

jnbrunet force-pushed the remove_eigen_from_within_sofa branch from f94e704 to 1e88c3f Compare March 26, 2020 16:43

hugtalbot mentioned this pull request Mar 27, 2020

ADD eigen as an official dependency sofa-framework/doc#33

Closed

guparan and others added 3 commits March 30, 2020 21:28

[SofaEigen2Solver] FIX dep macro naming

e7de4eb

SOFAEIGEN2SOLVER_WITH_OPENMP --> SOFAEIGEN2SOLVER_HAVE_OPENMP

[SofaEigen2Solver] FIX dep macro naming (2)

a9df959

SOFAEIGEN2SOLVER_WITH_OPENMP --> SOFAEIGEN2SOLVER_HAVE_OPENMP

[SofaEigen2Solver] Use cmakedefine instead of cmakedefine01

9076524

guparan reviewed Mar 31, 2020

View reviewed changes

jnbrunet removed request for fjourdes and matthieu-nesme March 31, 2020 14:44

jnbrunet added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Mar 31, 2020

damienmarchal reviewed Apr 1, 2020

View reviewed changes

hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Apr 1, 2020

jnbrunet added 2 commits April 1, 2020 16:03

[SofaEigen2Solver] Now use cmakedefine01 and #if instead of #ifdef pr…

b09f03b

…eprocessor tokens

[CGALPlugin] Reset the old FindCGAL.cmake

6c611d7

jnbrunet added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 2, 2020

jnbrunet requested a review from guparan April 3, 2020 09:07

guparan approved these changes Apr 3, 2020

View reviewed changes

guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 3, 2020

guparan merged commit c300240 into sofa-framework:master Apr 3, 2020

hugtalbot mentioned this pull request Apr 13, 2020

Setup doc: Eigen confusion sofa-framework/doc#34

Closed

guparan added this to the v20.06 milestone Jun 16, 2020

jnbrunet deleted the remove_eigen_from_within_sofa branch December 10, 2020 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. #1281

[cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. #1281

jnbrunet commented Mar 23, 2020 •

edited

Loading

guparan left a comment

guparan Mar 23, 2020

guparan Mar 23, 2020

jnbrunet Mar 23, 2020

guparan Mar 23, 2020

guparan Mar 23, 2020 •

edited

Loading

guparan Mar 23, 2020

guparan Mar 23, 2020

guparan Mar 23, 2020

jnbrunet commented Mar 24, 2020

guparan Mar 24, 2020

guparan Mar 25, 2020

guparan Mar 25, 2020

guparan Mar 26, 2020

jnbrunet commented Mar 26, 2020

guparan Mar 31, 2020

jnbrunet Mar 31, 2020

jnbrunet Mar 31, 2020

guparan Mar 31, 2020

jnbrunet Mar 31, 2020 •

edited

Loading

jnbrunet Mar 31, 2020 •

edited

Loading

guparan Mar 31, 2020

jnbrunet Mar 31, 2020

guparan Apr 1, 2020

jnbrunet commented Mar 31, 2020

damienmarchal Apr 1, 2020

hugtalbot commented Apr 1, 2020

jnbrunet commented Apr 2, 2020

jnbrunet commented Apr 2, 2020

guparan commented Apr 3, 2020

[cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. #1281

[cmake] Remove the use of an internal Eigen3 version and instead use the one installed on the system. #1281

Conversation

jnbrunet commented Mar 23, 2020 • edited Loading

guparan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guparan Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnbrunet commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnbrunet commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnbrunet Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

jnbrunet Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnbrunet commented Mar 31, 2020

Choose a reason for hiding this comment

hugtalbot commented Apr 1, 2020

jnbrunet commented Apr 2, 2020

jnbrunet commented Apr 2, 2020

guparan commented Apr 3, 2020

jnbrunet commented Mar 23, 2020 •

edited

Loading

guparan Mar 23, 2020 •

edited

Loading

jnbrunet Mar 31, 2020 •

edited

Loading

jnbrunet Mar 31, 2020 •

edited

Loading