-
Notifications
You must be signed in to change notification settings - Fork 311
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
[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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😉
@@ -3,9 +3,19 @@ | |||
@PACKAGE_INIT@ | |||
|
|||
set(SOFACOMMON_TARGETS @SOFACOMMON_TARGETS@) | |||
set(SOFAEIGEN2SOLVER_WITH_OPENMP @SOFAEIGEN2SOLVER_WITH_OPENMP@) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standard naming is SOFAEIGEN2SOLVER_HAVE_OPENMP
find_package(Eigen3 REQUIRED) | ||
|
||
# OpenMP might be required by SofaEigen2Solver | ||
if (SOFAEIGEN2SOLVER_WITH_OPENMP AND NOT TARGET OpenMP::OpenMP_CXX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standard naming is SOFAEIGEN2SOLVER_HAVE_OPENMP
Why AND NOT TARGET OpenMP::OpenMP_CXX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
SofaKernel/SofaCommon/config.h.in
Outdated
@@ -94,5 +94,7 @@ | |||
# define SOFA_EIGEN2_SOLVER_API SOFA_IMPORT_DYNAMIC_LIBRARY | |||
#endif | |||
|
|||
#cmakedefine SOFAEIGEN2SOLVER_WITH_OPENMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#cmakedefine01 SOFAEIGEN2SOLVER_HAVE_OPENMP
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
if (SOFAEIGEN2SOLVER_WITH_OPENMP) | ||
target_link_libraries(${PROJECT_NAME} PUBLIC OpenMP::OpenMP_CXX) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOFAEIGEN2SOLVER_HAVE_OPENMP
cmake/Modules/FindEigen3.cmake
Outdated
find_path(EIGEN3_INCLUDE_DIR NAMES signature_of_eigen3_matrix_library | ||
HINTS | ||
${EIGEN3_ROOT} | ||
ENV EIGEN3_ROOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Targets are nice 👍
Thanks for your review @guparan ! I've made the changes. |
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails on CentOS... Any idea why @jnbrunet ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After spending too much time on this, I disabled the build test with -DCGAL_TEST_RUNS=TRUE
.
Result: CGALPlugin build and tests are OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I fixed this issue. No need to -DCGAL_TEST_RUNS=TRUE
👍
6d0016d
to
f94e704
Compare
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.
f94e704
to
1e88c3f
Compare
[ci-build][with-all-tests] |
SOFAEIGEN2SOLVER_WITH_OPENMP --> SOFAEIGEN2SOLVER_HAVE_OPENMP
SOFAEIGEN2SOLVER_WITH_OPENMP --> SOFAEIGEN2SOLVER_HAVE_OPENMP
SofaKernel/SofaCommon/config.h.in
Outdated
@@ -94,7 +94,7 @@ | |||
# define SOFA_EIGEN2_SOLVER_API SOFA_IMPORT_DYNAMIC_LIBRARY | |||
#endif | |||
|
|||
#cmakedefine01 SOFAEIGEN2SOLVER_HAVE_OPENMP | |||
#cmakedefine SOFAEIGEN2SOLVER_HAVE_OPENMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this you won't be able to do if(SOFAEIGEN2SOLVER_HAVE_OPENMP)
in the code.
Why then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guparan Where have you seen if(SOFAEIGEN2SOLVER_HAVE_OPENMP)
in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only used as a preprocessor token, as it should
#ifdef SOFAEIGEN2SOLVER_HAVE_OPENMP
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if #cmakedefine
is used instead of #cmakedefine01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks for the tests 👍
What happens if #cmakedefine01
is used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because the idea behind #cmakedefine01
is to permit the usage of #if
and if()
. #ifdef
surely becomes useless though.
[ci-build][with-all-tests] |
@@ -311,7 +303,7 @@ class EigenBaseSparseMatrix : public defaulttype::BaseMatrix | |||
void mult_MT( VectorEigen& result, const VectorEigen& data ) | |||
{ | |||
compress(); | |||
#ifdef _OPENMP | |||
#ifdef SOFAEIGEN2SOLVER_HAVE_OPENMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if (SOFAEIGEN2SOLVER_HAVE_OPENMP == 1)
#...
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) |
[ci-build][with-all-tests] |
@guparan and @damienmarchal it should be good now, if you want to double check |
Bye bye embedded Eigen ✋ |
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:
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
elseseems to be fine with recent versions of Eigen.This PR:
Reviewers will merge only if all these checks are true.