redundancy: Offer centralized visibility macros?

[EricCousineau-TRI]

At present, from the crystal ros2 workspace, there are 43 instances of visibility_control.h*:

$ cd ros2_ws/src
$ find . -name 'visibility_control.h*' | wc -l
43

Only one is the *.em template file; the others only seem differ by the license header and the macro prefixes.
I could understand wanting to use project-specific visibility if it changes from project to project, but if it doesn't, wouldn't it make more sense to centralize them?

ament_cmake seems like a good candidate, as it's a build dependency, and thus something like this:

#include <ament_cmake/visibility_control.h>

AMENT_EXPORT
AMENT_IMPORT
AMENT_PUBLIC
// etc.

seems fine to me?

Related comment:
ros2/rcpputils#4 (comment)

[dirk-thomas]

The visibility macros need to be package specific since each symbol visibility needs to be controlled when a library of a package is being built and not when headers from dependency packages are included.

Providing a template with a CMake function which expands the template for a specific package sounds like a good idea though.

[wjwwood]

You must use different macro names for each library, so it is not possible to reuse a single instance with a single set of macros (e.g. AMENT_EXPORT, AMENT_IMPORT, etc...).

See: ros2/ros2#37 for example as to why we have one that's a template.

[wjwwood]

CMake already provides this functionality, https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html.

Personally I prefer not relying on generated code. I see little difference in a boilerplate header and boiler plate CMake logic, except the header is more transparent.

[EricCousineau-TRI]

I'd prefer the auto-generated route, even if it's not CMake-shipped stuff. However, it is nice to know what headers are actually there.

That being said, boilerplate without traceability is fragile since it can easily get out of sync.
There are several different flavors already present in the source workspace, so people may end up copying old cruft rather than a newer variant. (I just chose one and ran with it.)

At a minimum, if boilerplate were to be kept in-tree rather than generated, it should state when it was generated, and possibly point to the template it was generated from and the receipe (and should specify a permalink to a pinned SHA, if available).

Along with other redundancy issues like ros2/ros_core_documentation#13, I'd be happy to experiment with this.

[wjwwood]

That being said, boilerplate without traceability is fragile since it can easily get out of sync.
There are several different flavors already present in the source workspace, so people may end up copying old cruft rather than a newer variant. (I just chose one and ran with it.)

They're all based on this: https://gcc.gnu.org/wiki/Visibility

Which I believe they all mention, e.g.:

https://github.com/ros2/rclcpp/blob/1f2904f980aaaa7ccbfb40d722795b2d9a55a9aa/rclcpp/include/rclcpp/visibility_control.hpp#L25-L26

I don't know how much variability there is between them, but it would be something to clean up if they're very different. However, at the end of the day if the header in each package is doing its job, it doesn't really matter if it is slightly different from one in a different package.

Normally I would be on board for deduplicating code and trying to avoid divergence, but the reality is that the methods for deduplicating the code are more complicated and confusing that the code itself, and also this file basically never changes and I don't expect it to change (the example it is based on from GCC's wiki is ancient itself).

Having documentation describing how to create this header for a new project would be nice to have. It's relatively straightforward though, just copy and then find replace the prefix for the macros to something appropriate for your package.

[EricCousineau-TRI]

Yup, you're right, they do all mention it:

$ find . -name 'visibility_control.h*' | xargs grep -l 'https://gcc.gnu.org/wiki/Visibility' | wc -l
43

Having documentation describing how to create this header for a new project would be nice to have.

Definitely! It would also be nice to see something ament- or ROS2-specific in that.
If possible, it'd be even nicer to just point to Ament documentation.

I will try something along those lines, and make a draft PR to rclcpp within the next week or so. If it's too complex with little value add, I'd gladly close out that PR and this issue.
Thanks!

[dirk-thomas]

I will try something along those lines, and make a draft PR to rclcpp within the next week or so.

@EricCousineau-TRI Are you still planing to work on this or can this ticket be closed?

[dirk-thomas]

Closing due to no further response.