[All] Improve extlibs integration #1137

guparan · 2019-09-05T13:22:44Z

This is a (big) attempt to clean the dependencies management in SOFA.

Better understand SOFA dependencies

Here is a summary: https://docs.google.com/spreadsheets/d/1dxP7Rsyl1oCr0lfTwQESeeZQTt315CIFih5QpJp5OZY/edit?usp=sharing

Clean dependency management in CMakeLists

SofaFramework
- Moved to SofaHelper: Boost, OpenGL, GLEW, GTest, Eigen, JSON, FFMPEG_exec
- Removed (not needed by any SofaFramework module): TinyXML, ZLIB
SofaMisc
- Added to SofaMiscTopology: ZLIB
- Added to SofaMiscFem: Newmat
SofaGeneral
- Added to SofaDenseSolver: Newmat
- Added to SofaGeneralLoader: ZLIB
SofaGui
- Moved to SofaGuiQt: Qt + lots of options about QtViewer, QGLViewer, VSync, ...
Modularized modules (oh yeah)
- SofaPreconditioner: removed Newmat, added SofaSparseSolver
- SofaExporter: added ZLIB
Plugins
- SofaMiscCollision: added SofaDistanceGrid
- SofaImplicitField: added SofaPython
- Registration: removed SofaOpenglVisual
- image: added ZLIB
- Flexible: added SofaPython

Rework all YYY_HAVE_XXX variables to reduce their scope

This is important for future global modularization of SOFA.

Variables are now named upon their module.
Whenever you need to use the YYY_HAVE_XXX of another module, you surely should declare your own MYMODULE_HAVE_XXX instead.
All those new YYY_HAVE_XXX variables are booleans, so they can be used in CMake with if(YYY_HAVE_XXX) and in C++ (if correcly propagated via a config.h.in file) with #if YYY_HAVE_XX or if(YYY_HAVE_XXX).
Add sofa_find_package and sofa_set_bool macros to auto create YYY_HAVE_XXX variables.
Edit sofa_install_targets to call configure_file() on any *.h.in found in target sources (the sources that are given to add_library()).

⚠️ Here is a quick tuto about how to correctly add and propagate a dependency.

Rework #1121 to avoid nested projects

Use SOFA_NO_OPENGL to build an OpenGL-free version of SofaGuiQt

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.

namespace alias "po" makes difficult to find where program_options is used

SofaTest_test contains only: - scenes/damping.py - python_tests/python_test_test.py If this scripts are needed, they should be in SofaPython_test.

sofa_set_int: define a variable to 0 or 1 to be usable by both config.h.and or XXXConfig.cmake.in sofa_extlib_find_package: call find_package + call sofa_set_int to create a XXX_HAVE_YYY variable

…ind_package

"name" has nothing to do as add_subdirectory 2nd argument

Useful to debug. Prints all properties of a target.

SofaHighOrderTopology is needed by Flexible

Because some variables defined above may be needed in the configured files

- Add forward declaration to TinyXML classes - Add find_package - Link TinyXML privately

Setting variables as boolean permits to use them with #if or directly with if() without implicit conversion warning.

Always using the same name from CMake option definition to C++ #ifdef blocks

The addition of TimeoutWatchdog is already controlled by Boost_THREAD_FOUND in CMakeLists.txt

fredroy · 2019-09-05T15:21:22Z

SofaGui/SofaGuiConfig.cmake.in

+        find_package(QGLViewer QUIET REQUIRED)
+    endif()
+    if(SOFAGUIQT_HAVE_QWT)
+        find_package(QGLViewer QUIET REQUIRED)


Typo ? find_package(qwt QUIET REQUIRED) instead ?

Thanks! 👍

SOFAHELPER_HAVE_FFMPEG_EXEC is a boolean

guparan · 2019-09-09T07:59:59Z

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

…libs_integration # Conflicts: # applications/sofa/gui/qt/CMakeLists.txt # applications/sofa/gui/qt/gl/CMakeLists.txt # applications/sofa/gui/qt/viewer/GLBackend.cpp # extlibs/CMakeLists.txt

even if variable goes from TRUE to UNDEFINED (old way let the #define with no value, leading to errors)

Better consistency with #cmakedefine01

guparan · 2019-09-24T09:07:33Z

modules/SofaExporter/src/SofaExporter/initExporter.cpp

@@ -60,11 +60,12 @@ void initExternalModule()
    if (first)
    {
        first = false;
-        if(SOFAEXPORTER_HAVE_SOFAPYTHON)
+#if SOFAEXPORTER_HAVE_SOFAPYTHON


Thanks, this was indeed broken.

guparan · 2019-09-24T09:30:29Z

modules/SofaExporter/CMakeLists.txt

+    option(SOFAEXPORTER_BUILD_TESTS "Compile the automatic tests" ON)
+else()
+    set(SOFAEXPORTER_BUILD_TESTS FALSE)
+endif()


I don't agree with this approach.
SOFA_BUILD_TEST is for SOFA tests. SofaExporter is now modularized so it should be considered as out-of-SOFA.

My view is:

SOFA old modules (that are not real modules) do not integrate their tests themselves because of SofaTest dependencies (see the order of add_subdirectory calls in main CMakeLists)

new modules ("modularized") and plugins and projects must be buildable out of SOFA so they integrate their tests themselves and do not depend on a SOFA internal variable. Those tests, though, can have a SofaTest dependency (SofaExporter_test does) and thus may fail to generate if SofaTest is not found.

In this case for example, I prefer an explicit SOFAEXPORTER_BUILD_TESTS that may lead to a failure because SofaExporter_test couldn't find SofaTest than a magic check above SofaExporter_test setting the variable OFF beforehand.

What could be changed though is the way SofaExporter_test finds SofaTest

find_package(SofaTest QUIET) if(NOT SofaTest_FOUND) message(SEND_ERROR "Couldn't find SofaTest. If building SOFA, you need to activate SOFA_BUILD_TESTS") endif()

ok I understand that tests from module should be independant but they need SofaTest lib.
In my case I don't compile SofaTest (bad I know). But unchecking SofaTest failed at cmake configuration because SOFAEXPORTER_BUILD_TESTS need SofaTest. So it means I would need to uncheck each TEST in the cmake configuration...

Maybe the good approach is (not sure it works):

if (SOFA_BUILD_TESTS) option(SOFAEXPORTER_BUILD_TESTS "Compile the automatic tests" ON) else() option(SOFAEXPORTER_BUILD_TESTS "Compile the automatic tests" OFF) endif()

and in the SOFAExporter_test cmake:

find_package(SofaTest REQUIRED) if(NOT SofaTest_FOUND) message(SEND_ERROR "Couldn't find SofaTest. If building SOFA, you need to activate SOFA_BUILD_TESTS") endif()

I think the problem behind is that SOFA_BUILD_TESTS should not be used to check SofaTest_FOUND.
SOFA_BUILD_TESTS should be only used to ON/OFF the old module tests (all at once).

Being able to ON/OFF specific tests is a good thing in my opinion.
Here is a compromise, in SofaExporter_test CMakeLists:

find_package(SofaTest QUIET) if(NOT SofaTest_FOUND) message(STATUS "Couldn't find SofaTest. ${PROJECT_NAME} will not be built.") return() endif()

This is more or less the same idea but keeping the check inside of SofaExporter_test.

…package(SofaTest)

epernod

Not sure it works fully as expected but for me cmake generation works as well as compilation and make install.
I was also able to create a project using SOFA as lib. It is not totally easy going but it works.

guparan added 30 commits July 23, 2019 14:55

[CMake] CLEAN Sofa*Targets.cmake inclusions and SofaComponent* packages

70c9067

[All] CLEAN Boost::program_options usage

d0ab5f6

namespace alias "po" makes difficult to find where program_options is used

[SofaTest_test] Disable project

4ae2521

SofaTest_test contains only: - scenes/damping.py - python_tests/python_test_test.py If this scripts are needed, they should be in SofaPython_test.

[gtest] CLEAN include directories

c9f5e03

[MiniFlowVR] CLEAN include directories

4717ed0

[Newmat] CLEAN Config.cmake

bc5f2d2

[TinyXML] CLEAN Config.cmake

6fb9260

[SofaGTestMain] CLEAN CMakeLists.txt

80ac32e

[All] Remove useless code

a7cae05

[CMake] FIX SceneCreator and SofaTest build dirs

7e71af8

[SofaMacros] ADD sofa_set_int and sofa_extlib_find_package

45cb6c8

sofa_set_int: define a variable to 0 or 1 to be usable by both config.h.and or XXXConfig.cmake.in sofa_extlib_find_package: call find_package + call sofa_set_int to create a XXX_HAVE_YYY variable

[SofaAdvanced] Remove MiniFlowVR (not used)

6a57316

[SofaMacros] ADD BOTH_SCOPES option to sofa_set_int and sofa_extlib_f…

5876fad

…ind_package

[SofaMacros] FIX add_subdirectory in sofa_add_generic

917a521

"name" has nothing to do as add_subdirectory 2nd argument

[SofaMacros] Rename sofa_extlib_find_package to sofa_find_package

75aa01a

[SofaMacros] ADD debug_print_target_properties

29b79e2

Useful to debug. Prints all properties of a target.

[plugins] Handle SofaHighOrder first

6da48e7

SofaHighOrderTopology is needed by Flexible

[libQGLViewer] CLEAN CMake OpenGL link

89401fd

[CMake] MOVE configure_file to the end in package CMakeLists

f9ebec8

Because some variables defined above may be needed in the configured files

[SofaSimulationGraph] MOVE test inclusion to modules/tests

7f0c399

[SofaSimulationGraph] FIX missing config.h

41b1af7

[SofaSimulationGraph_test] CLEAN CMakeLists.txt

e80cb57

[SofaSimulationCommon] CLEAN TinyXML dependency

7976a9f

- Add forward declaration to TinyXML classes - Add find_package - Link TinyXML privately

[SofaMacros] Change sofa_set_int to sofa_set_bool

0f4e61d

Setting variables as boolean permits to use them with #if or directly with if() without implicit conversion warning.

[SofaHaptics] Remove unneeded SOFA_HAVE_SOFAHAPTICS

2fdfb7d

[SofaComponentMisc] CLEAN CMakeLists.txt

08c5815

[All] CLEAN usage of SOFA_DETECTIONOUTPUT_FREEMOTION

d5f5289

Always using the same name from CMake option definition to C++ #ifdef blocks

[SofaHelper] Remove unneeded SOFA_HAVE_BOOST

2ab529a

The addition of TimeoutWatchdog is already controlled by Boost_THREAD_FOUND in CMakeLists.txt

[LeapMotion] Remove unneeded boost include

20fcfbf

[SofaOpenglVisual] FIX Config.cmake.in

e331195

fredroy reviewed Sep 5, 2019

View reviewed changes

guparan added 10 commits September 5, 2019 17:22

[SofaMacros] Propagate sofa_set_bool fix to sofa_find_package

19a8efd

[SofaGui] FIX typo in Qwt finding + improve

8e6f9fb

[SofaCUDA] FIX missing config.h

c6da587

[CGALPlugin] FIX missing config.h

5a3a807

[SofaCUDA] FIX includes order

b1a034f

[SofaGui] FIX #ifdef to #if for SOFAHELPER_HAVE_FFMPEG_EXEC

fad8f73

SOFAHELPER_HAVE_FFMPEG_EXEC is a boolean

[Modeler] FIX TinyXML dependency

8c59d5b

[SofaMiscFem_test] FIX missing test

13b0a1d

[SofaSimulationGraph_test] FIX missing test

36593c5

[CMake] Change order to have SofaSparseSolver when building tests

a0f79b1

Merge remote-tracking branch 'sofa-framework/master' into improve_ext…

b75790b

…libs_integration # Conflicts: # applications/sofa/gui/qt/CMakeLists.txt # applications/sofa/gui/qt/gl/CMakeLists.txt # applications/sofa/gui/qt/viewer/GLBackend.cpp # extlibs/CMakeLists.txt

guparan 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 Sep 17, 2019

guparan added 2 commits September 17, 2019 17:58

[All] Use cmakedefine01 to ensure correct value

0dda89a

even if variable goes from TRUE to UNDEFINED (old way let the #define with no value, leading to errors)

[CMake] RENAME sofa_set_bool to sofa_set_01

8f3ef32

Better consistency with #cmakedefine01

guparan 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 Sep 17, 2019

epernod added 2 commits September 24, 2019 10:45

[cmake] FIX cmake configuration when SOFA_TEST is disabled

fbd9356

[SofaExporter] FIX compilation when sofa_python is not activated.

eca0439

guparan commented Sep 24, 2019

View reviewed changes

[cmake] update condition to activate specific package test. Use find_…

d2cd22f

…package(SofaTest)

epernod approved these changes Sep 24, 2019

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 Sep 25, 2019

guparan merged commit 5654a0e into sofa-framework:master Sep 26, 2019

guparan mentioned this pull request Sep 26, 2019

[packages] Move all SofaComponent* + rename SofaAllCommonComponents #1155

Merged

6 tasks

guparan added this to the v19.12 milestone Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[All] Improve extlibs integration #1137

[All] Improve extlibs integration #1137

guparan commented Sep 5, 2019 •

edited

Loading

fredroy Sep 5, 2019

guparan Sep 5, 2019

guparan commented Sep 9, 2019

guparan Sep 24, 2019

guparan Sep 24, 2019 •

edited

Loading

epernod Sep 24, 2019 •

edited

Loading

guparan Sep 24, 2019

epernod left a comment

[All] Improve extlibs integration #1137

[All] Improve extlibs integration #1137

Conversation

guparan commented Sep 5, 2019 • edited Loading

Better understand SOFA dependencies

Clean dependency management in CMakeLists

Rework all YYY_HAVE_XXX variables to reduce their scope

Rework #1121 to avoid nested projects

fredroy Sep 5, 2019

Choose a reason for hiding this comment

guparan Sep 5, 2019

Choose a reason for hiding this comment

guparan commented Sep 9, 2019

guparan Sep 24, 2019

Choose a reason for hiding this comment

guparan Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

epernod Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

guparan Sep 24, 2019

Choose a reason for hiding this comment

epernod left a comment

Choose a reason for hiding this comment

guparan commented Sep 5, 2019 •

edited

Loading

guparan Sep 24, 2019 •

edited

Loading

epernod Sep 24, 2019 •

edited

Loading