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

Add rosidl_get_typesupport_target and deprecate rosidl_target_interfaces #606

Merged
merged 3 commits into from
Aug 4, 2021

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 27, 2021

This adds rosidl_get_typesupport_target() which returns the name of a typesupport target. This can be used in a target_link_libraries() call to make a target depend on generated code for interfaces generated in the same CMake project.

This PR also deprecates rosidl_target_interfaces() which inconveniently wraps target_link_libraries() and has what appears to be extra unnecessary add_dependencies() and target_include_directories() calls.

Adding rosidl_get_typesupport_target() is in support of ros2/python_cmake_module#6 , specifically replacing a good bit of FindPythonExtra with Python3_add_library(). Python3_add_library() calls target_link_libraries() internally on the given target with a Keyword argument, but that conflicts with rosidl_target_interfaces() which uses the non-keyword version of target_link_libraries() internally. CMake won't allow those two styles to mix on the same target (without using the OLD policy of CMP0023). This PR allows more control over which version of target_link_libraries() is used, and that should allow rosidl_generator_py to use Python3_add_library()

Also deprecate rosidl_target_interfaces().
Getting the target name allows consumers to choose the keyword arguments
that they pass to target_link_libraries()

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Jul 27, 2021

CI (repos: https://gist.githubusercontent.com/sloretz/055a0c8759a9e9bc66272fffbfc07ac3/raw/3432eb38980450e2f179b18c62ee5b9d0b72b30e/rosidl_get_typesupport_target.repos branch: use_rosidl_get_typesupport_target build: --packages-above-and-dependencies rosidl_cmake test: --packages-above rosidl_cmake)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Jul 27, 2021

CI (repos: https://gist.githubusercontent.com/sloretz/055a0c8759a9e9bc66272fffbfc07ac3/raw/23f2eec68bc404604dccbefcbfb6d97057b8886c/rosidl_get_typesupport_target.repos branch: use_rosidl_get_typesupport_target build: --packages-above-and-dependencies rosidl_cmake test: --packages-above rosidl_cmake)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Jul 28, 2021

Rebuild of CI - hopefully github is working this morning.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Jul 28, 2021

Warning on windows is caused by ros2/rclcpp#1731

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but I'll approve anyway.

CI is green, but that is dependent on a number of other changes so downstream packages don't start spitting warnings. I'll assume you'll merge all of them at once.


set("${var}" "${output_target}" PARENT_SCOPE)
endfunction()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra trailing space at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the extra newline in 0f158d7

@clalancette
Copy link
Contributor

Oh, can I also request that you add a changelog note to https://github.com/ros2/ros2_documentation/blob/rolling/source/Releases/Release-Humble-Hawksbill.rst , since this will cause a bunch of warnings on packages outside of the core?

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Aug 4, 2021

Added a note to the changelog in ros2/ros2_documentation#1798

@sloretz
Copy link
Contributor Author

sloretz commented Aug 4, 2021

CI (repos: https://gist.githubusercontent.com/sloretz/055a0c8759a9e9bc66272fffbfc07ac3/raw/23f2eec68bc404604dccbefcbfb6d97057b8886c/rosidl_get_typesupport_target.repos branch: use_rosidl_get_typesupport_target build: --packages-up-to rosidl_cmake sensor_msgs test: --packages-select rosidl_cmake sensor_msgs) - limited CI since only a whitespace change. rosidl_cmake checks linting, while sensor_msgs is a double check that the new function works.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Aug 4, 2021

CI Green and all linked PRs approved. I'll coordinate merging all at once with the libstatistics_collector maintainers.

@sloretz sloretz merged commit b3e3e90 into master Aug 4, 2021
@delete-merged-branch delete-merged-branch bot deleted the use_rosidl_get_typesupport_target branch August 4, 2021 23:08
@clalancette
Copy link
Contributor

@sloretz The nightlies went yellow from this. It looks like there is one more use of the deprecated rosidl_target_interfaces: https://github.com/ros2/common_interfaces/blob/22206cdddf9d65c6ad271778f11047cf65592823/sensor_msgs/CMakeLists.txt#L73

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

Successfully merging this pull request may close these issues.

2 participants