Cleanup visibility to enable compilation on Windows

[traversaro]

I would like to compile and run the software of this repo on Windows. I was able to do so with https://github.com/ros-controls/ros2_controllers/pull/1039 and https://github.com/ros-controls/ros2_control/compare/master...traversaro:ros2_control:patch-2 . However, given the comments in https://github.com/ros-controls/ros2_controllers/pull/1039#discussion_r1485948944, I thought it made sense to open an issue to discuss what is the preferred way to handle visibility of symbols on Windows.

Describe the solution you'd like

Differently from Linux and macOS, on Windows by default the symbols of shared libraries are not exported. To deal with this, there are several strategies:

• S1: Emulate on Windows the behavior of Linux/macOS, i.e. export all symbols from the shared library. This can be done on Windows by setting the CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS. The upside is the only change necessary to support a shared library CMake project on Windows is to add a set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) cmake line, no other change is necessary beside some specific corner cases. The downside of this is all symbols (even the private one) are exported, but this also applies also on Linux and macOS.
• S2: If one does not want to export all the symbols in a shared library, one can instead manually export each symbols that needs to be exported. To do so, project-specific export macros need to be appropriately defined. An header that defines these macros can be automatically generated by CMake using the generate_export_header CMake macro.
• S3: A variant of S2 is to manually annotated each symbol that needs to be exported, but using a manually written header that is committed to the repo, instead of generating it an header via generate_export_header.

Unless the shared library is particularly big, I typically prefer to adopt S1, as it is the strategy that put the least possible burden of library maintainers and authors, especially if most of them is not familiar with Windows. If S2 or S3 are used, I personally prefer at least to make sure that the visibility is hidden by default also on Linux and macOS, to permit developers to make sure that the visibility macros are working appropriately even without using Windows.

For some reasons (some discussed in https://github.com/ament/ament_cmake/issues/201#issuecomment-542808202 and https://github.com/ament/ament_cmake/issues/164) core ros2 packages use the S3 strategy, even if only a subset of packages actually set the visibility to hidden in non-Windows platforms (see https://github.com/search?q=org%3Aros2+hidden+language%3ACMake&type=code). Apparently the S3 strategy is used also on ros2_control repos.

Given this discussion, I would like to know what maintainers prefer:

• Either to continue with S3 strategy, and fixing the existing errors (i.e. cleaning up https://github.com/ros-controls/ros2_controllers/pull/1039 and opening a PR for https://github.com/ros-controls/ros2_control/compare/master...traversaro:ros2_control:patch-2)
• Or change strategy, if they think that the gain in maintainability make it worth to change strategy.

[christophfroehlich]

Thanks for this detailed explanation: Finally I understand why this is necessary at all (ping https://github.com/ros-controls/ros2_control_demos/issues/73 :wink:) I vote for S2 + default hidden, but I'm not a qualified software engineer.

fyi: I added this to the agenda of the working group meeting next week, let's discuss it there if no one comments here until then.

[traversaro]

fyi: I added this to the agenda of the working group meeting next week, let's discuss it there if no one comments here until then.

Ok, I will try to attend so we can discuss/explain if anyone has specific questions, thanks!

[destogl]

I would go with S3 and fix all the issues we currently have.

[traversaro]

I would go with S3 and fix all the issues we currently have.

I kind of have already patches for this, see:

• https://github.com/ros-controls/ros2_controllers/compare/master...traversaro:ros2_controllers:steeringexport
• https://github.com/ros-controls/ros2_control/compare/master...traversaro:ros2_control:patch-2

The tricky aspect is that the classes have a lot of protected methods, that technically are part of the public interface. I added patches for all the protected methods (as for example on_set_chained_mode needs to be exported) while I did not exported any protected attribute, that worked but I am not sure if that was just luck or it is kind of aligned with the actual usage.

[bmagyar]

The discussion on this topic at the WG meeting yesterday concluded with Option S1.

Does the S stand for Silvio? ;) @traversaro

[traversaro]

Does the S stand for Silvio? ;) @traversaro

I had to think a bit about it. :D I think it wanted to stand for "Solution".

[traversaro]

As S1 was decided, I realized that there are a few decision to make:

• Even if the macros are empty on Linux, removing them (and similarly, removing the visibility headers) is technically a breaking change. Are you ok in removing them in rolling or you prefer to keep the header but empty?

[christophfroehlich]

sidenote: I'd remove them from the demos soon, because we don't release the demos repos and can easily get rid of the boilerplate code there for every distro.

[christophfroehlich]

@traversaro did you have time to work on this? as we are preparing more breaking stuff on rolling, now it's the time to break even more ;)

[traversaro]

@traversaro did you have time to work on this? as we are preparing more breaking stuff on rolling, now it's the time to break even more ;)

Cool! With summer coming I hope to be able to prepare some PRs on this, thanks for the ping!