Misc cleanup in the rosidl generator extensions #662

sloretz · 2022-02-17T23:24:21Z

This PR cleans up a few things I've noticed while looking at the generator code for another task

Make sure to find_package() dependencies needed by generated code
Don't wrap the target_compile_definitions(...BUILDING_DLL...) line in an if(WIN32) block because it's unnecessary. The visibility header seems to do stuff on other platforms too.
Instead of depending on all targets in a dependency interface package, depend on just the targets generated by the same generator using the suffixed .._TARGETS${_target_suffix} variable created by rosidl_export_typesupport_targets().
- also use rosidl_export_typesupport_targets() where it wasn't already
Remove if(NOT _generated_headers... checks - because they don't seem to do anything. In all places I saw them there was another place further up or down that wasn't guarded and would have raised a CMake error if that variable actually was empty (either the add_library() or add_custom_target() commands).
Use target_link_libraries() instead of ament_target_dependencies() Is ament_target_dependencies() still necessary? ament/ament_cmake#292 in the generator code (There are a few other places it's used in this repo still, but they weren't part of the generators so I didn't include them in this PR)
Replace checks on rosidl_generator_c_FOUND with checks that the target that generator generates is found - because that's the important thing.
- Note to self: generators can control what order their extension is called by find_packaging() the generators they depend on before registering their own extensions in their "extras.cmake" file
When a generator depends on another, make it target_link_libraries() to the generated target of the other rather than hard-coding the directory it's likely to create in the build folder
Avoid calling ament_export_dependencies(...) when SKIP_INSTALL is used.

sloretz · 2022-02-17T23:38:33Z

I don't see these failures in my focal container, but the PR job is the first one ever on Jammy https://build.ros2.org/job/Rpr__rosidl__ubuntu_jammy_amd64/1 . I'll try making a Jammy container and reproducing there.

sloretz · 2022-02-18T00:22:14Z

I get the same test failures on Jammy, with and without this PR - so they're not caused by it. Still, those will need to be fixed for green CI.

sloretz · 2022-02-18T19:51:25Z

CI (build: --packages-up-to rcl_interfaces test: --packages-select rosidl_generator_c rosidl_generator_cpp rosidl_typesupport_introspection_cpp rosidl_typesupport_introspection_c rcl_interfaces)

Linux
Linux-aarch64
Windows

sloretz · 2022-02-22T17:11:41Z

@ros-pull-request-builder retest this please

clalancette

I did a review of this, and I left two comments inline. Both of them are really questions since I'm not that familiar with how all of this works. It may be a good idea to get another review from someone like @hidmic before going ahead with this.

clalancette · 2022-02-22T18:55:03Z

rosidl_generator_c/cmake/rosidl_generator_c_generate_interfaces.cmake

-  if(NOT rosidl_generate_interfaces_SKIP_INSTALL)
-    ament_export_dependencies(${_pkg_name})
-  endif()


This is my first time looking at this code, so take this with a grain of salt. If we stop calling ament_export_dependencies for the recursive dependencies, can't we get into a situation where we aren't including the correct targets? That is, if msg package A depends on msg package B, shouldn't it express that dependency to things depending on A? Or is there another mechanism that will cause that to automatically happen?

rosidl_generate_interfaces() already calls ament_export_dependencies() on these. This removes rosidl_generator_c doing it an extra time.

clalancette · 2022-02-22T18:59:45Z

rosidl_generator_cpp/cmake/rosidl_generator_cpp_generate_interfaces.cmake

+  ${rosidl_generate_interfaces_TARGET}
+  ${rosidl_generate_interfaces_TARGET}${_target_suffix})


It's unclear to me why we need both of these. Could you explain a bit more (and maybe add a comment)?

When a package calls rosidl_generate_interfaces(foobar they're passing in a name to create for a top-level interface generation target. That target name is passed to the individual generators in the variable ${rosidl_generate_interfaces_TARGET}. Each generator creates their own target to do message generation, and they all do the same pattern where they define a variable called _target_suffix and then make their generator specific target with the name ${rosidl_generate_interfaces_TARGET}${_target_suffix}. The second add_dependencies() target is making the top level target depend on the generator specific target. That way, any other target that depends on the top level target will wait until all generators finish before being built. That's part of how targets can use generated messages in the same package.

rosidl_generator_cpp only generates headers, so it's library target is a header only library, which in CMake is an INTERFACE library. CMake doesn't provide a way for an INTERFACE library to directly depend on a file, so there's nothing to make targets who depend on the INTERFACE library wait for the header files to be generated. To solve this, rosidl_generator_cpp creates an extra custom target called ${rosidl_generate_interfaces_TARGET}__cpp. CMake does allow custom targets to depend on generated files, and that's all this one does. The first add_dependency() makes the generator's INTERFACE library depend on that custom target so that anyone who depends on the library will wait to be built until the headers are generated.

Added comments explaining these in 8a433e9

sloretz · 2022-03-08T19:10:38Z

CI again (build: all test: --packages-select rosidl_generator_c rosidl_generator_cpp rosidl_typesupport_introspection_cpp rosidl_typesupport_introspection_c rcl_interfaces rcl)

Linux
Linux-aarch64
Windows

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

hidmic

Only a couple nits. Overall LGTM

hidmic · 2022-03-15T22:38:29Z

rosidl_generator_c/cmake/rosidl_generator_c_generate_interfaces.cmake

-    PRIVATE "ROSIDL_GENERATOR_C_BUILDING_DLL_${PROJECT_NAME}")
-endif()
+target_compile_definitions(${rosidl_generate_interfaces_TARGET}${_target_suffix}
+  PRIVATE "ROSIDL_GENERATOR_C_BUILDING_DLL_${PROJECT_NAME}")


@sloretz nit: ROSIDL_GENERATOR_C_BUILDING_DLL_* macros are only used in visibility headers on Windows (e.g. see here).

Oh oops. I read that header wrong.

Interestingly CMake has a target property for this already called DEFINE_SYMBOL. It adds the compiler definition <target_name>_EXPORTS by default, setting the property changes it. I changed it to use that property instead in 519066b

hidmic · 2022-03-15T22:42:54Z

rosidl_generator_cpp/cmake/rosidl_generator_cpp_generate_interfaces.cmake


  install(
    TARGETS ${rosidl_generate_interfaces_TARGET}${_target_suffix}
-    EXPORT ${rosidl_generate_interfaces_TARGET}${_target_suffix}
+    EXPORT export_${rosidl_generate_interfaces_TARGET}${_target_suffix}


@sloretz nit: why add an export_ suffix here but not for rosidl_generator_c?

I'm inconsistent. Added export_ in 368d022

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2022-03-17T00:29:38Z

CI re-run

Linux
Linux-aarch64
Windows

hidmic · 2022-03-17T16:56:09Z

...pesupport_introspection_c/cmake/rosidl_typesupport_introspection_c_generate_interfaces.cmake

-    PRIVATE "ROSIDL_TYPESUPPORT_INTROSPECTION_C_BUILDING_DLL_${PROJECT_NAME}")
-endif()
+target_compile_definitions(${rosidl_generate_interfaces_TARGET}${_target_suffix}
+  PRIVATE "ROSIDL_TYPESUPPORT_INTROSPECTION_C_BUILDING_DLL_${PROJECT_NAME}")


@sloretz shall we use DEFINE_SYMBOL here too?

hidmic · 2022-03-17T16:56:24Z

...pport_introspection_cpp/cmake/rosidl_typesupport_introspection_cpp_generate_interfaces.cmake

-    PRIVATE "ROSIDL_TYPESUPPORT_INTROSPECTION_CPP_BUILDING_DLL")
-endif()
+target_compile_definitions(${rosidl_generate_interfaces_TARGET}${_target_suffix}
+  PRIVATE "ROSIDL_TYPESUPPORT_INTROSPECTION_CPP_BUILDING_DLL")


@sloretz same about using DEFINE_SYMBOL

hidmic · 2022-03-17T16:57:31Z

rosidl_generator_c/cmake/rosidl_generator_c_generate_interfaces.cmake

-    PRIVATE "ROSIDL_GENERATOR_C_BUILDING_DLL_${PROJECT_NAME}")
-endif()
+set_property(TARGET ${rosidl_generate_interfaces_TARGET}${_target_suffix}
+  PROPERTY DEFINE_SYMBOL "ROSIDL_GENERATOR_C_BUILDING_DLL_${PROJECT_NAME}")


@sloretz unconditionally? I'm OK with that, though talking about DLLs outside Windows is awkward.

Looks that way. If you build rcl with -DCMAKE_VERBOSE_MAKEFILE=ON all the compiler invocations have -Drcl_EXPORTS passed to them; which is the default value of DEFINE_SYMBOL.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2022-03-17T18:09:59Z

CI re-run

Linux
Linux-aarch64
Windows

sloretz self-assigned this Feb 17, 2022

sloretz force-pushed the sloretz__misc_generator_cleanup branch from 71ba58a to eb98af1 Compare February 18, 2022 19:46

clalancette reviewed Feb 22, 2022

View reviewed changes

This was referenced Feb 24, 2022

Misc fastrtps typesupport generator cleanup ros2/rosidl_typesupport_fastrtps#87

Merged

Misc rosidl_typesupport cleanup ros2/rosidl_typesupport#123

Merged

sloretz requested a review from hidmic March 8, 2022 19:11

sloretz mentioned this pull request Mar 8, 2022

Use rosidl_get_typesupport_target and target_link_libraries ros2/rmw_dds_common#57

Merged

sloretz added 3 commits March 14, 2022 13:59

Misc cleanup in the rosidl generator extensions

131583d

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Fix typo in error message

e1b4c02

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add more comments about why dependencies are added

c43822e

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the sloretz__misc_generator_cleanup branch from 8a433e9 to c43822e Compare March 14, 2022 21:00

This was referenced Mar 15, 2022

Add sensor_msgs_library target and install headers to include/${PROJECT_NAME} ros2/common_interfaces#178

Merged

Install generated headers to include/${PROJECT_NAME} #670

Merged

hidmic approved these changes Mar 15, 2022

View reviewed changes

sloretz added 2 commits March 15, 2022 17:31

Prefix export file name with export_

368d022

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

use DEFINE_SYMBOL property to add compile definition on Windows

519066b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

hidmic reviewed Mar 17, 2022

View reviewed changes

Use DEFINE_SYMBOL in more places

ea3a3ca

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

hidmic approved these changes Mar 21, 2022

View reviewed changes

sloretz merged commit 0861738 into master Mar 21, 2022

delete-merged-branch bot deleted the sloretz__misc_generator_cleanup branch March 21, 2022 14:58

Blast545 mentioned this pull request Apr 28, 2022

Add missing test dependency to package.xml file #678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc cleanup in the rosidl generator extensions #662

Misc cleanup in the rosidl generator extensions #662

sloretz commented Feb 17, 2022 •

edited

Loading

sloretz commented Feb 17, 2022

sloretz commented Feb 18, 2022

sloretz commented Feb 18, 2022

sloretz commented Feb 22, 2022

clalancette left a comment

clalancette Feb 22, 2022

sloretz Feb 22, 2022

clalancette Feb 22, 2022

sloretz Feb 22, 2022

sloretz Feb 22, 2022

sloretz commented Mar 8, 2022

hidmic left a comment

hidmic Mar 15, 2022

sloretz Mar 16, 2022

hidmic Mar 15, 2022

sloretz Mar 16, 2022

sloretz commented Mar 17, 2022

hidmic Mar 17, 2022

sloretz Mar 17, 2022

hidmic Mar 17, 2022

sloretz Mar 17, 2022

hidmic Mar 17, 2022

sloretz Mar 17, 2022

sloretz commented Mar 17, 2022

		${rosidl_generate_interfaces_TARGET}
		${rosidl_generate_interfaces_TARGET}${_target_suffix})

Misc cleanup in the rosidl generator extensions #662

Misc cleanup in the rosidl generator extensions #662

Conversation

sloretz commented Feb 17, 2022 • edited Loading

sloretz commented Feb 17, 2022

sloretz commented Feb 18, 2022

sloretz commented Feb 18, 2022

sloretz commented Feb 22, 2022

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Mar 8, 2022

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Mar 17, 2022

sloretz commented Feb 17, 2022 •

edited

Loading