Remove boilerplate visibility macros #1972
Conversation
github-actions
bot
requested review from
anfemosa,
aprotyas,
arne48,
christophfroehlich,
fmauch,
peterdavidfagan and
saikishor
There was a problem hiding this comment.
Aren't these parts in the CMakeLists.txt now outdated as well?
# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(controller_interface PRIVATE "CONTROLLER_INTERFACE_BUILDING_DLL")
|
Excellent point! On to removing more stuff!! |
|
I've also removed the cmake macro for controller interface which may break controllers... but I'll put up a PR for that too |
| macro(controller_interface_configure_controller_library library_target) | ||
| if(WIN32) | ||
| # Causes the visibility macros to use dllexport rather than dllimport | ||
| # which is appropriate when building the dll but not consuming it. | ||
| target_compile_definitions(${library_target} | ||
| PRIVATE "CONTROLLER_INTERFACE_BUILDING_DLL") | ||
| endif() | ||
| endmacro() |
There was a problem hiding this comment.
LOL. I didn't know that this macro exist
There was a problem hiding this comment.
The changes itself look good to me, but this might be API breaking as we are removing the headers.
For ros2_controllers it is not a problem as no one derives from them, but for ros2_control it might be.
What do you guys think? @ros-controls/ros2-maintainers ?
|
That's why we have the downstream CI jobs. I think we can work with the UR driver maintainers ;) |
Ok, we will check with the UR drivers. I'm just worried about user's code that is not released but in their local workspace. If this is not a problem at all, then perfect 😜 |
There was a problem hiding this comment.
should we first deprecate this for one release cycle?
ros-controls/ros2_controllers#1053