[Windows] dllexport support for ament_cmake

[seanyen]

On Windows, Microsoft C/C++ compiler comes with a concept of dllexport and dllimport. In order to make a DLL library header able to be consumed by the downstream project, the function, class, or data declaration needs to be decorated with the correct attributes.

CMake provides CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and attempts to fix the problem less intrusive to the existing code base. However, it still comes with limitation, for example, it doesn't support static member, and it will bloat the size of the generated imported library because it exhaustively exposes whatever it can do.

In ROS1, I explored a way to enable dllexport and dllimport for a catkin-based project. Here is the pull request for rviz to show the example. This change makes use of generate_export_header and with some careful steps to stage and install it to a place to be consumed within and across catkin packages.

I would like to use this ticket to discuss:

1. Any feedback on the generate_export_header approach?
2. Would it make sense to make it part of ament_cmake requirements or should it be just step-by-step guidance in the ament_cmake User Documentation.

[seanyen]

ros2/teleop_twist_joy#14

[seanyen]

ros-perception/vision_opencv#301

[mjcarroll]

Those two referenced PRs are now in.

I'm generally in favor of this, I think it makes more sense than our current approach of placing _visibility.hpp in our vcs, and makes for a more consistent implementation across the packages.

[wjwwood]

This has been discussed before: #164

In summary, I prefer the boilerplate header we use (e.g. https://github.com/ros2/rclcpp/blob/231b991098d685ff019c44edb80a8ff170d65e4a/rclcpp/include/rclcpp/visibility_control.hpp#L25-L54) to the boilerplate generated code that cmake has to generate every time you build. Neither change, but the static one is nicer in my opinion because you don't waste time generating it each time and because it lets static code analysis (or like autocomplete for text editors) work without having to first build the package and have the build/install space for that package on the path for your tool. Also, if you were to port a package to a different build system (e.g. bazel or something) then you don't have to also port the behavior of generate_export_header.

[wjwwood]

Any feedback on the generate_export_header approach?

It's fine, but as I said above I prefer just committing a single header per library.

Would it make sense to make it part of ament_cmake requirements or should it be just step-by-step guidance in the ament_cmake User Documentation.

I don't see what this has to do with ament or ament_cmake. From my perspective, it's a Windows and CMake thing, we should just link to documentation about it that's external to our project. What exactly would you put into ament_cmake?

[traversaro]

In summary, I prefer the boilerplate header we use (e.g. https://github.com/ros2/rclcpp/blob/231b991098d685ff019c44edb80a8ff170d65e4a/rclcpp/include/rclcpp/visibility_control.hpp#L25-L54) to the boilerplate generated code that cmake has to generate every time you build. Neither change, [..]

Just to clarify, the existing boilerplate header and the functionality provided by the generate_export_header CMake function are not completely equivalent. In the case a library is built as a static library in MSVC (by setting BUILD_SHARED_LIBS to OFF), then you do not want your exported symbols to be decorated as __declspec(dllimport), as otherwise you will have linking error in the executable that is using the static library (see JuliaStrings/utf8proc#96, google/glog#155 and ros2/rclcpp#638 (comment) as an example of related issues).

A non-generated header roughly equivalent to generate_export_header would be something like:

#if defined _WIN32 || defined __CYGWIN__
  #ifdef __GNUC__
    #define RCLCPP_EXPORT __attribute__ ((dllexport))
    #define RCLCPP_IMPORT __attribute__ ((dllimport))
  #else
    #define RCLCPP_EXPORT __declspec(dllexport)
    #define RCLCPP_IMPORT __declspec(dllimport)
  #endif 
  #ifndef RCLCPP_STATIC_LIBRARY
    #ifdef RCLCPP_BUILDING_LIBRARY_SHARED
      #define RCLCPP_PUBLIC RCLCPP_EXPORT
    #else
      #define RCLCPP_PUBLIC RCLCPP_IMPORT
    #endif
  #else 
    #define RCLCPP_PUBLIC 
  #endif 
  #define RCLCPP_PUBLIC_TYPE RCLCPP_PUBLIC
  #define RCLCPP_LOCAL
#else
  #define RCLCPP_EXPORT __attribute__ ((visibility("default")))
  #define RCLCPP_IMPORT
  #if __GNUC__ >= 4
    #define RCLCPP_PUBLIC __attribute__ ((visibility("default")))
    #define RCLCPP_LOCAL  __attribute__ ((visibility("hidden")))
  #else
    #define RCLCPP_PUBLIC
    #define RCLCPP_LOCAL
  #endif
  #define RCLCPP_PUBLIC_TYPE
#endif

And the following CMake code will be necessary for the library, to correctly pass the compilation flags:

set_target_properties(${PROJECT_NAME} PROPERTIES DEFINE_SYMBOL RCLCPP_BUILDING_LIBRARY_SHARED)
if(BUILD_SHARED_LIBS)
  target_compile_definitions(${PROJECT_NAME}
    PUBLIC RCLCPP_STATIC_LIBRARY)
endif()

As the definition of the RCLCPP_STATIC_LIBRARY is encoded in the CMake configuration files instead of a generated header, downstream projects that do not use CMake need to manually define RCLCPP_STATIC_LIBRARY in all the compilation units that use the static rclcpp.

[traversaro]

Another example of well defined visibility macro header is the one reported in https://gcc.gnu.org/wiki/Visibility, but in the "Step-by-step guide" section instead of the "How to use the new C++ visibility support" section that is the one that is manually copied in almost all ROS2 packages and described in https://index.ros.org/doc/ros2/Tutorials/Ament-CMake-Documentation/#building-libraries-on-windows .

[wjwwood]

We could update our visibility headers to support the static library case. However, we need to probably touch each one anyways to properly support that, and so it's not much of a burden. At the very least we'd have to manage the use of the compiler definitions that indicate static library on windows, and also though we've built some/most of the stack static for Android in the past, I'm not sure what the current state of it is. It's not an officially supported use case or at least not one that we test regularly.