[cmake] Simplify cmake files for build-check and non-regression testing #88
Conversation
…sofa.create.target
…A plugins for build consistency
th-skam
added
pr: enhancement
pr: status to review
fredroy
left a comment
fredroy
left a comment
There was a problem hiding this comment.
(maybe in an other PR)
and set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) should be removed as well. And use *_API macro mechanism to select which symbols to be exported
CMakeLists.txt
Outdated
| "deprecated/*.inl" | ||
| "deprecated/*.cpp" | ||
| ) | ||
| file(GLOB_RECURSE HEADER_FILES "src/*.h" "src/*.inl") |
There was a problem hiding this comment.
It is not really advised to use GLOB_RECURSE for sources files
https://cmake.org/cmake/help/latest/command/file.html#glob
Note
We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate. The CONFIGURE_DEPENDS flag may not work reliably on all generators, or if a new generator is added in the future that cannot support it, projects using it will be stuck. Even if CONFIGURE_DEPENDS works reliably, there is still a cost to perform the check on every rebuild.
CMakeLists.txt
Outdated
| INCLUDE_SOURCE_DIR "src" | ||
| INCLUDE_INSTALL_DIR ${PROJECT_NAME} | ||
| EXAMPLE_INSTALL_DIR "scenes" | ||
| ) |
There was a problem hiding this comment.
Usually plugins must set "RELOCATABLE"
| ) | |
| RELOCATABLE "plugins" | |
| ) |
| #include <CollisionAlgorithm/BaseElement.h> | ||
| #include <thread> | ||
|
|
||
| namespace sofa::collisionAlgorithm { |
There was a problem hiding this comment.
not related to the PR, but I would take the opportunity while refactoring/breaking stuff, to change the namespace (AFAIK there is not any namespace in camelCase)
…cessary directives
There was a problem hiding this comment.
So this has been handy when we only want collision without insertion (e.g. needle vs bones).
I'll probably absorb this in the new algorithm but since I have all relevant scenes set up this way I'm keeping it around for the time being.
… classes into the new architecture
|
|
||
| #ifdef SOFA_BUILD_COLLISIONALGORITHM | ||
| # define SOFA_TARGET @PROJECT_NAME@ | ||
| # define SOFA_COLLISIONALGORITHM_API SOFA_EXPORT_DYNAMIC_LIBRARY |
There was a problem hiding this comment.
This is the good way but I saw that it is just used in the init.cpp files. I think you will need to use it to export the classes as well (or the explicit instanciations if they are templated).
But if you are not on Windows, it will be a bit difficult to do it "blindly" so it may be the best to do it in a other PR and test it on Windows.
There was a problem hiding this comment.
Thanks a lot for these tips @fredroy!
A question about using this feature properly in SOFA plugins:
I would be using the *_API macro to export classes in the following situations:
- For base classes that I might want to inherit from and extend in another plugin
- Derived ones that will end up in a sofa scene as components
- Some internal classes (like an iterator) that are returned by public functions and should be exposed
and limit the export right there. Would you recommend this approach or perhaps export classes in other cases as well?
epernod
left a comment
epernod
left a comment
There was a problem hiding this comment.
If all approve, I'll do the same :)
epernod
added
pr: status ready
This PR simplifies the
CMakeLists.txtandCollisionAlgorithmConfig.cmake.infiles.This is to be more consistent with the structure of other SOFA plugins (e.g.
BeamAdapter) and to ease the build-checks in the current and theConstraintGeometryrepositories, as well as the non-regression testing being currently built.