[core] samples and tests moved to ecal folder #1419
Conversation
ecal/CMakeLists.txt
Outdated
| add_subdirectory(service) | ||
|
|
||
| if(BUILD_ECAL_TESTS) |
There was a problem hiding this comment.
in this file, we will use only options prefixed with ECAL_CORE -> we need option(ECAL_CORE_BUILD_TESTS ON "build ecal tests") or something similar. -> I am open for the naming, but it should be consitent for the subcomponents. In eCAL itself we have not been 100% consistent.
From Main CMakeLists.txt you will then do
if (BUILD_ECAL_TESTS)
set(ECAL_CORE_BUILD_TESTS ON)
endif()
| add_subdirectory(cpp/services/ping_client_dyn) | ||
| add_subdirectory(cpp/services/ping_server) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
To be honest, this is nuts. Too many options, too many combinations, too hard to maintain.
Maybe we can find another way for the samples to build them only if their specified component is available?
Doing all of that with CMake/preprocessor directives only is so error prone.
Maybe we can think more in the component direction. Just to keep thinking about it.
| @@ -40,4 +40,4 @@ target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14) | |||
|
|
|||
| ecal_install_sample(${PROJECT_NAME}) | |||
|
|
|||
| set_property(TARGET ${PROJECT_NAME} PROPERTY FOLDER samples/c/pubsub/string) | |||
| set_property(TARGET ${PROJECT_NAME} PROPERTY FOLDER core/samples/c/pubsub/string) | |||
There was a problem hiding this comment.
I think CMake is actually able to automate this. However, I think for the user it might be more convenient to actually group by test first, and then the component?
There was a problem hiding this comment.
You could try using CMAKE_FOLDER and do string(APPEND CMAKE_FOLDER "/<scope>") at the top for every directory. Would be pretty tedious to go through and change everything over though. Upside being you don't need to call set_property(TARGET for every target however.
| @@ -33,7 +33,7 @@ int main(int argc, char **argv) | |||
|
|
|||
| // create subscriber "Hello" | |||
| sub = eCAL_Sub_New(); | |||
| eCAL_Sub_Create(sub, "Hello", "std::string", "base", "", 0); | |||
| eCAL_Sub_Create(sub, "Hello", "base", "std::string", "", 0); | |||
There was a problem hiding this comment.
Are you sure about this? (apparently yes, but does it make sense for the API?) How do we construct the SDatatypeInformation Objects? SDatatypeInformation(name, encoding, descriptor).
So I think we should change the eCAL_Sub_Create signature to reflect that.
|
|
||
| source_group(TREE "${CMAKE_CURRENT_SOURCE_DIR}" FILES | ||
| ${${PROJECT_NAME}_src} | ||
| ) |
There was a problem hiding this comment.
In the future, we may consider building different parts of eCAL (pub, sub, ...) as object or static libraries, so that we can test their interfaces better and don't have to list and build all sources again as done here.
rex-schilasky
requested review from
FlorianReimold
and removed request for
FlorianReimold
Description
All ecal-core sample and test c/c++ projects are now inside the ecal core subdirectory. This allow easy alignment with ecal-core project and may lead to an integration as submodule finally.
Cherry-pick to