Add `visibility_control.h` to the API as standard usage

Next Generation ROS
1

The visibility_control.{h/hpp} is present in most of the projects. Why isn’t it simply added to the RCL API to avoid replication?

Is there a reason for this file to be copy-pasted in each project with renamed macros or is it bad practice because it was not present at first?

find ~/ros2/src -name visibility_control.h* | wc -l
58
2

that last bit would be why the file gets copied: those macros are project-specific.

3

But these macros, apart from being renamed, are identical.

The rcutils library should probably be the one included as it defines:

* Macros for controlling symbol visibility and linkage for this library:
  * rcutils/visibility_control.h
4

Yes, they are identical, except the name.

As in some cases, you’d need to be able to control how specific projects either import or export symbols.

Note how it says: “for this library”.

I’m not saying the situation cannot be improved. I just answered your question: why are these files copied?

(we could consider creating another layer of macros that only takes a name and then specialises a template, another possibility could be to use CMake’s GenerateExportHeader module (but there may be requirements/constraints that made this impossible/undesirable for the ROS2 codebase))

5

Unfortunately, due to Windows linkage, it really does need to be per-project.

When you build a library on Windows, all symbols are private (meaning nothing can call them). For each symbol that you want public and callable by the outside world, you need to mark them with dllexport during compile time of the library. When you want a downstream consumer to use them, you need to mark them with dllimport during compile time of the downstream consumer. So far, this is all pretty clear from the macros.

The trouble comes when you want library A to depend on library B. In that case, you need to mark the symbols of library B as dllimport (so library A can call them), but you need to mark the public symbols of library A as dllexport. So you can’t just have one set of macros to accomplish this. This is why there are per-project macros.

We’ve traditionally shied away from using GenerateExportHeader because it locks you into using CMake as your build system, while separate visibility macros don’t. I honestly don’t know if there is substantial interest in building ROS packages without CMake, so I can’t comment on whether that is a good enough reason or not. (There’s also my personal bias that GenerateExportHeader is “magic”, while the hand-coded macros are straightforward to read, but I can get over that).

1 Like
6

Exactly.

Probably (not). In any case, replacing an entire build system seems like it would be more involved than just worrying about some generated files.

Wouldn’t it be possible to create a template macro that wraps all the visibility macros and takes a single argument: an identifier (which would take values such as RCUTILS, RCLCPP, etc).

Calling the macro results in the template being expanded which turns it into the classical content of a visibility_control.h header, but for a specific package.

The macro could be shared among all packages, any language with a C-compatible preprocessor could use it (so no build system specific functionality required) and it would avoid the duplication @AlexisTM asks about.

1 Like
7

Yeah, with token pasting I think this would work. Assuming there are no other problems that others have with this scheme, I’d be happy to review patches to the core to add this functionality.

8

The independence of the build system is that the driving factor why we are currently having separate headers.

The reason we kept the header files in the source of each package is that IDEs as well as static analysis tools are able to find the header and know about these macros. When being generated by the build system that is commonly an issue since the header isn’t present in the source space of the package.

See [Windows] dllexport support for ament_cmake · Issue #201 · ament/ament_cmake · GitHub for a longer discussion around this topic.

1 Like
9

@clalancette (and others) great explanation. I’m not involved at all in this but just want to put in a vote for documentation: can this rationale be copied by someone (@AlexisTM?) as comments at the top of some/all of the visibility_control.h files to avoid revisiting this topic by others in the future?

10

so you want to even further increase the amount of duplicated content? :wink:

11

People build ROS 1 with bazel at least.

That’s probably also true.

I have to admit that the current state is at least partially due to my preferences, because I prefer not to have to generate files if it can be avoided. As Dirk mentioned, it makes it easier to integrate into IDEs and state code analyzers, but the reality is that we generate code in other places anyways (though at the time that we added the visibility headers we didn’t have any other generated code, aside from messages).

Despite my preference to the contrary, if many others want it to be generated then that’s fine by me, but it does seem like a sideways move to me.

12

Half of this header is dealing with GCC/MSVC difference of syntax. We could at least extract that out and avoid duplicating this logic everywhere, by factoring it out in rcutils.

In rcutils:

#if defined _WIN32 || defined __CYGWIN__
# define RCUTILS_DLL_IMPORT __declspec(dllimport)
# define RCUTILS_DLL_EXPORT __declspec(dllexport)
# define RCUTILS_DLL_LOCAL
#else
# if __GNUC__ >= 4
#  define RCUTILS_DLL_IMPORT __attribute__ ((visibility ("default")))
#  define RCUTILS_DLL_EXPORT __attribute__ ((visibility ("default")))
#  define RCUTILS_DLL_LOCAL  __attribute__ ((visibility ("hidden")))
# else
#  define RCUTILS_DLL_IMPORT
#  define RCUTILS_DLL_EXPORT
#  define RCUTILS_DLL_LOCAL
# endif
#endif

Duplicated code in every other package (here rmw_connext_cpp):

#include "rcutils/visibility_control_macros.h"
#ifdef RMW_CONNEXT_CPP_BUILDING_DLL
# define RMW_CONNEXT_CPP_PUBLIC RCUTILS_DLL_EXPORT
#else
# define RMW_CONNEXT_CPP_PUBLIC RCUTILS_DLL_IMPORT
#endif
#define RMW_CONNEXT_CPP_LOCAL RCUTILS_DLL_LOCAL

Less duplicated code, still no generated header, and as this is a new header, this does not break existing code, people can refactor their package if they want later.

4 Likes
13

I just went and sent a PR for this: https://github.com/ros2/rcutils/pull/194

14

While your solution certainly works to deduplicate code, this brings another burden to the user. Now, every package has to depend on rcutils. In order to use the macro, every package has to set the dependencies straight in their package.xml and CMakeLists.txt.

The current situation is not ideal right now, but it’s essentially one search-and-replace command to get the visibility macros to compile with your package.

EDIT: One thing to mention is that ros2 pkg create creates that visibility header for you. So there is actually no need to copy paste anything in the first place when starting a new project.