-
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
[All] Improve extlibs integration #1137
[All] Improve extlibs integration #1137
Conversation
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
"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
SofaGui/SofaGuiConfig.cmake.in
Outdated
find_package(QGLViewer QUIET REQUIRED) | ||
endif() | ||
if(SOFAGUIQT_HAVE_QWT) | ||
find_package(QGLViewer QUIET REQUIRED) |
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.
Typo ? find_package(qwt QUIET REQUIRED) 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.
Thanks! 👍
SOFAHELPER_HAVE_FFMPEG_EXEC is a boolean
[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
@@ -60,11 +60,12 @@ void initExternalModule() | |||
if (first) | |||
{ | |||
first = false; | |||
if(SOFAEXPORTER_HAVE_SOFAPYTHON) | |||
#if SOFAEXPORTER_HAVE_SOFAPYTHON |
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.
Thanks, this was indeed broken.
option(SOFAEXPORTER_BUILD_TESTS "Compile the automatic tests" ON) | ||
else() | ||
set(SOFAEXPORTER_BUILD_TESTS FALSE) | ||
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.
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()
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.
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()
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.
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)
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 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.
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
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
orif(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 toadd_library()
).Rework #1121 to avoid nested projects
This PR:
Reviewers will merge only if all these checks are true.