Misc rosidl_typesupport cleanup

[sloretz]

This cleans up the rosidl generator code in the packages rosidl_typesupport_c and rosidl_typesupport_cpp

• rosidl_typesupport_c only
  • Remove redundant visibility header Misc rosidl_typesupport cleanup #123 (comment)
• rosidl_typesupport_cpp only
  • Add missing dependency on rcutils
• rosidl_typesupport_c and rosidl_typesupport_cpp
  • Replace ament_target_dependencies with target_link_libraries() Is ament_target_dependencies() still necessary? ament/ament_cmake#292
  • use DEFINE_SYMBOL for setting the compiler definition that toggles the visibility export macros.
  • Depend on the target generated by rosidl_generator_c or rosidl_generator_cpp, instead of hard coding include directories that they're going to create
  • When depending on another interface package, depend only on the target this generator created for that interface package instead of depending on all targets
  • Make sure to ament_export_dependencies() the packages needed by generated targets
  • Remove _generator_sources check that can't work because other CMake code would error if it were ever actually empty

depends on ros2/rosidl#666 Merged 🎉
depend on ros2/rosidl#662 Merged 🎉

[sloretz]

@hidmic I added you as a reviewer, but this one seems to depend on ros2/rosidl#662 so I've left it as a draft until that one is merged.

[hidmic]

@sloretz nit: same comment as in ros2/rosidl#662 about this definition being unnecessary outside Windows.

[hidmic]

@sloretz mind to explain why visibility control is no longer necessary? Specifically why dllexport and dllimport attributes are no longer necessary on Windows anymore.

[sloretz]

It's not that they're not necessary, but that there's already a visibility file for this symbol created by rosidl_generator_c. This generator creates the .cpp file which has the symbol in a cpp file declared with the export macro from this file

rosidl_typesupport/rosidl_typesupport_c/resource/msg__type_support.cpp.em

Lines 128 to 130 in 97bc0a7

	ROSIDL_TYPESUPPORT_C_EXPORT_@(package_name)
	const rosidl_message_type_support_t *
	ROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join([package_name] + list(interface_path.parents[0].parts))), @(message.structure.namespaced_type.name))() {

But the header included by downstream packages is generated by rosidl_generator_c, and that uses a different visibility header file also generated by rosidl_generator_c

https://github.com/ros2/rosidl/blob/08617387920b40b8aa6b48db90d3535090be6086/rosidl_generator_c/resource/msg__type_support.h.em#L12-L16

https://github.com/ros2/rosidl/blob/master/rosidl_generator_c/resource/rosidl_generator_c__visibility_control.h.in

See how it's included in the detail/interface__type_support.h header generated by rosidl_generator_c.

https://github.com/ros2/rosidl/blob/08617387920b40b8aa6b48db90d3535090be6086/rosidl_generator_c/resource/idl__type_support.h.em#L28

Currently this generator has to set both compiler definitions because the .cpp generated by rosidl_typesupport_c file includes that typesupport header generated by rosidl_generator_c. If it didn't, the compiler would complain about the header saying dllimport while the .cpp file says dllexport.

rosidl_typesupport/rosidl_typesupport_c/cmake/rosidl_typesupport_c_generate_interfaces.cmake

Lines 103 to 106 in 97bc0a7

	target_compile_definitions(${rosidl_generate_interfaces_TARGET}${_target_suffix}
	PRIVATE "ROSIDL_GENERATOR_C_BUILDING_DLL_${PROJECT_NAME}")
	target_compile_definitions(${rosidl_generate_interfaces_TARGET}${_target_suffix}
	PRIVATE "ROSIDL_TYPESUPPORT_C_BUILDING_DLL_${PROJECT_NAME}")

This PR cleans it up by removing the visibility header generated by rosidl_typesupport_c. The header generated by rosidl_generator_c already has the symbol with an export macro, and the .cpp file includes it, so the .cpp file doesn't need it again. The result is it uses the visibility header generated by rosidl_generator_c.

[sloretz]

This had a missing piece. I assumed the rosidl_generator_c generated visibility header was included when building the .cpp file, but that wasn't the case. 57c6c7a fixes that.

[sloretz]

CI (build: --packages-above-and-dependencies rosidl_typesupport_c rosidl_typesupport_cpp test: --packages-above-and-dependencies rosidl_typesupport_c rosidl_typesupport_cpp)

• Linux
• Linux-aarch64
• Windows

[sloretz]

CI failure involves rosidl_generator_py. I'll look into why it failed with this PR:

15:23:26 LINK : fatal error LNK1181: cannot open input file 'Release\rosidl_generator_py_custom__rosidl_typesupport_c.lib' [C:\ci\ws\build\rosidl_generator_py\rosidl_generator_py_custom__rosidl_generator_py.vcxproj]

[sloretz]

CI re-run

• Linux
• Linux-aarch64
• Windows

[sloretz]

CI LGTM 🎉 The windows warning is in rmw_fastrtps and was fixed by ros2/rmw_fastrtps#592

[cottsay]

I successfully used this change during testing on RHEL and verified that it substantially reduces the unnecessary linking observed in our message packages.

Thanks for the much-needed cleanup.