ros-controls / ros2_control_demos

This repository aims at providing examples to illustrate ros2_control and ros2_controllers
https://control.ros.org
Apache License 2.0
412 stars 187 forks source link

Remove visibility macros #539

Closed christophfroehlich closed 3 months ago

christophfroehlich commented 3 months ago

As a consequence of https://github.com/ros-controls/ros2_control/pull/1627 there is an include for HARWARE_INTERFACE_PUBLIC missing, causing the build of example_7 to fail.

As we've discussed and decided in https://github.com/ros-controls/ros2_controllers/issues/1053, we wanted to remove all the visibility macros and use set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) instead. I took the chance to do so here.

Fixes #538 and also resolves #73

christophfroehlich commented 3 months ago

:eyes: @traversaro

henrygerardmoore commented 3 months ago

Fixes build for me locally, but shouldn't lines such as

# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(${PROJECT_NAME} PRIVATE "ROS2_CONTROL_DEMO_EXAMPLE_1_BUILDING_DLL")

be removed? This is lines 39-41 of example 1's CMakeLists but every example has similar lines

christophfroehlich commented 3 months ago

Ah, thanks for the review. I missed that

christophfroehlich commented 3 months ago

@traversaro do you know if we need target_compile_definitions or similar for windows builds?

traversaro commented 3 months ago

@traversaro do you know if we need target_compile_definitions or similar for windows builds?

If you do not have any visibility macro, it is ok not to add any target_compile_definitions. As a general comment (in case someone reads this) also if you have the visibility macro it is not ideal to have code like:

target_compile_definitions(${PROJECT_NAME} PRIVATE "ROS2_CONTROL_DEMO_EXAMPLE_1_BUILDING_DLL")

as this will define the macro even if the target ${PROJECT_NAME} is a static library. The correct property to set is the DEFINE_SYMBOL (https://cmake.org/cmake/help/latest/prop_tgt/DEFINE_SYMBOL.html) of the target, or even better just use the default DEFINE_SYMBOL value that is <targetName>_EXPORTS .

christophfroehlich commented 3 months ago

Changes seems ok to me, thanks! As raised in the PR review, you can also remove the target_compile_definitions(${PROJECT_NAME} PRIVATE "ROS2_CONTROL_DEMO_EXAMPLE_7_BUILDING_DLL") code, but if there are no visibility headers that code is basically ignored.

Thanks a lot for the review, I'll remove that dead code then ;)