Ros2 reenable tests #59
Conversation
mikaelarguedas
force-pushed
the
ros2_reenable_tests
branch
from
7e4e265 to
31bbf7c
Compare
test/fviz_case_study/CMakeLists.txt
Outdated
| ament_add_gtest(${PROJECT_NAME}_fviz_test fviz_test.cpp) | ||
| set(append_library_dirs "${CMAKE_CURRENT_BINARY_DIR}") | ||
| if(WIN32) | ||
| set(append_library_dirs "${append_library_dirs}/../../$<CONFIG>;${append_library_dirs}/$<CONFIG>") |
There was a problem hiding this comment.
@wjwwood @dirk-thomas this is pretty ugly. Am I approaching this the wrong way?
There was a problem hiding this comment.
I get why you want to use ${append_library_dirs}/$<CONFIG> but where is ${append_library_dirs}/../$<CONFIG> supposed to be pointing to?
Maybe you can use a generator expression to point to the target directory of one of the plugin libraries instead (like $<TARGET_FILE_DIR:${PROJECT_NAME}_TestPlugins1>)?
src/class_loader.cpp
Outdated
| return ".dll"; | ||
| #else | ||
| return Poco::SharedLibrary::suffix(); |
There was a problem hiding this comment.
Wouldn't it be nice if the debug shared libraries could be loaded for debugging?
test/CMakeLists.txt
Outdated
| @@ -7,43 +7,62 @@ target_include_directories(${PROJECT_NAME}_TestPlugins1 | |||
| PUBLIC "../include" ${console_bridge_INCLUDE_DIRS} ${Poco_INCLUDE_DIRS}) | |||
| target_link_libraries(${PROJECT_NAME}_TestPlugins1 ${PROJECT_NAME}) | |||
| class_loader_hide_library_symbols(${PROJECT_NAME}_TestPlugins1) | |||
|
|
|||
| if(WIN32) | |||
| target_compile_definitions(${PROJECT_NAME}_TestPlugins1 PRIVATE "CLASS_LOADER_BUILDING_DLL") | |||
There was a problem hiding this comment.
My expectation would be that this should only be defined when class_loader is being built. Why is this necessary here? And if it is I would suggest to use a separate name to indicate that the "using" part of class loader is being built here.
There was a problem hiding this comment.
Without this, the tests linking against these testing libraries fail to link.
And if it is I would suggest to use a separate name to indicate that the "using" part of class loader is being built here.
I'm not sure I get what you mean here, do you suggest creating another set of visibility macros for the libraries built and used for testing?
| add_dependencies(${PROJECT_NAME}_utest ${PROJECT_NAME}_TestPlugins1 ${PROJECT_NAME}_TestPlugins2) | ||
| add_dependencies(${PROJECT_NAME}_utest | ||
| ${PROJECT_NAME}_TestPlugins1 | ||
| ${PROJECT_NAME}_TestPlugins2) |
There was a problem hiding this comment.
Since the target already links against these libraries the dependency declaration is redundant.
Same below.
There was a problem hiding this comment.
indeed the target should not link these libraries, fixed in 08d4345
test/fviz_case_study/CMakeLists.txt
Outdated
| ament_add_gtest(${PROJECT_NAME}_fviz_test fviz_test.cpp) | ||
| set(append_library_dirs "${CMAKE_CURRENT_BINARY_DIR}") | ||
| if(WIN32) | ||
| set(append_library_dirs "${append_library_dirs}/../../$<CONFIG>;${append_library_dirs}/$<CONFIG>") |
There was a problem hiding this comment.
I get why you want to use ${append_library_dirs}/$<CONFIG> but where is ${append_library_dirs}/../$<CONFIG> supposed to be pointing to?
Maybe you can use a generator expression to point to the target directory of one of the plugin libraries instead (like $<TARGET_FILE_DIR:${PROJECT_NAME}_TestPlugins1>)?
mikaelarguedas
force-pushed
the
ros2_reenable_tests
branch
from
0e2753d to
343c625
Compare
…15 so we cant use that, removing all cpp11 checks for now given that ros2 and melodic will only target compilers that support it
mikaelarguedas
force-pushed
the
ros2_reenable_tests
branch
from
343c625 to
210ea9f
Compare
|
Ok I think this is ready for a round of reviews This PR reenables the tests for ROS2, convert the tests using catkin and boost to use ament and STL and modifies test amd CMake code to pass on all platforms
Note: I removed all the checks for c++11 support, my assumption here is that I'll branch out class_loader for Melodic and will not need that check in Melodic either because all compilers will support c++11 by default. I will reconciliate the melodic-devel and the ros2 branches at that point. |
|
ok I rerun a full set of CI and everything's green, merging |
Opening for visibility.
Apparently class_loader tests have been disabled in ROS2 for a looong time (ament/ament_cmake#68). This aims to reenable and fix them