search

ros2 / common_interfaces

A set of packages which contain common interface files (.msg and .srv).
225 stars 106 forks source link

Modern CMake Targets #216

Open Leon0402 opened 1 year ago

Leon0402 commented 1 year ago

I would appreciate modern cmake targets, so things like ament_target_dependencies don't have to be used anymore (see https://github.com/ament/ament_cmake/issues/292). It seems modern cmake targets are produced already by many interfaces such as std_msgs. For instance: std_msgs::std_msgs__rosidl_typesupport_cpp

But I saw some interfaces define a nicer wrapper library for this: For instance https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/CMakeLists.txt#L67 (Although an alias target should be added as well for this).

So I wonder what the general strategy is. It seems a little bit inconsistent. I personally don't like these generated names such as std_msgs::std_msgs__rosidl_typesupport_cpp. So I would be in favor of having wrapper targets. On the other hand, it seems like this could be implemented directly in rosidl_generate_interfaces and similar functions?

clalancette commented 1 year ago

Yes, the goal is definitely to have std_msgs::std_msgs as a target. We attempted to do this at one point, but ran into some issues. @sloretz do you happen to remember what problems we had?

Anyway, if you have some ideas on how to make this nicer, we'd be happy to consider them.

sloretz commented 1 year ago

There is a variable with all the CMake targets: ${<package name>_TARGETS}, ex: ${std_msgs_TARGETS}. There isn't a wrapper target at the moment. I don't remember what problems were had with it.

Leon0402 commented 1 year ago

Anyway, if you have some ideas on how to make this nicer, we'd be happy to consider them.

Well I mean the approach that was taken seems like a first improvement. The PR for this was https://github.com/ros2/common_interfaces/pull/178#issuecomment-1076516076. Which also mentions a few issues, but seems like they all were resolved. For my own library I use similar code currently.

But yeah in general the ideal solution would be to have a method in ros which does all the heavy work. Or if the existing methods like rosidl_generate_interfaces would just define the target itself. The new method rosidl_get_typesupport_target already helps a little bit, but I don't see a reason why some ros method shouldn't do the target definition as well.

Ryanf55 commented 1 year ago

This duplicates https://github.com/ros2/rosidl/issues/746.