Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modern CMake Targets #216

Open
Leon0402 opened this issue Mar 6, 2023 · 4 comments
Open

Modern CMake Targets #216

Leon0402 opened this issue Mar 6, 2023 · 4 comments
Labels

Comments

@Leon0402
Copy link

Leon0402 commented Mar 6, 2023

I would appreciate modern cmake targets, so things like ament_target_dependencies don't have to be used anymore (see ament/ament_cmake#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
Copy link
Contributor

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
Copy link
Contributor

sloretz commented Mar 6, 2023

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
Copy link
Author

Leon0402 commented Mar 7, 2023

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 #178 (comment). 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
Copy link
Contributor

Ryanf55 commented Sep 13, 2023

This duplicates ros2/rosidl#746.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants