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

Misc fastrtps typesupport generator cleanup #87

Merged
merged 10 commits into from
Mar 24, 2022

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 23, 2022

This cleans up a bunch of stuff to do with the generators for rosidl_typesupport_fastrps_c and rosidl_typesupport_fastrtps_cpp

  • Make sure to find_package() dependencies needed by generated code
  • Made rosidl_typesupport_fastrtps_cpp find_package rosidl_generator_cpp so it generates files first
  • Avoid calling ament_export_dependencies(...) when SKIP_INSTALL is used.
    • This means moving a few calls out of CMakeLists.txt and into the generator's CMake 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.
  • Add checks that generated targets exist from the generators these generators depend on
  • Use target_link_libraries() instead of ament_target_dependencies() Is ament_target_dependencies() still necessary? ament/ament_cmake#292 in a couple places
  • 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
  • Remove an unnecessary dependency between the target generated by rosidl_typesupport_fastrtp_c and the target generated by rosidl_typesupport_fastrtps_cpp
  • When target_link_libraries() on targets from this generator in dependency packages, use the ..._TARGETS${_target_suffix} variable instead of the ..._LIBRARIES${_target_suffix}
    • 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
  • Reviewed and organized the package.xmls with comments

Depends on ros2/rmw_dds_common#57

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz self-assigned this Feb 23, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Feb 23, 2022

CI (build: all test: --packages-select rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp test_msgs rcl_interfaces)

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

@sloretz sloretz marked this pull request as draft February 23, 2022 01:29
@sloretz
Copy link
Contributor Author

sloretz commented Feb 24, 2022

Marked as draft while I open other PRs. So far in my local testing it seems fixing these issues requires fixing up cmake issues in rmw_dds_common which requires fixing up cmake issues in rosidl_typesupport_c which requires the cmake fixes in ros2/rosidl#662 .

@sloretz
Copy link
Contributor Author

sloretz commented Feb 24, 2022

Depends on ros2/rmw_dds_common#57

@sloretz sloretz requested a review from hidmic March 8, 2022 19:16
@sloretz
Copy link
Contributor Author

sloretz commented Mar 8, 2022

@hidmic I've added you as a reviewer, but this one depends on ros2/rmw_dds_common#57 which depends on ros2/rosidl_typesupport#123 which depends on ros2/rosidl#662, so I've left it marked as a draft until those are merged.

sloretz and others added 4 commits March 15, 2022 17:08
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
…ithub.com:ros2/rosidl_typesupport_fastrtps into sloretz__rosidl_typesupport_fastrtps_cmake_issues
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM once this comes out of draft mode

@sloretz
Copy link
Contributor Author

sloretz commented Mar 22, 2022

Leaving in draft until ros2/rosidl_typesupport#123 and then ros2/rmw_dds_common#57 are merged.

@sloretz sloretz marked this pull request as ready for review March 24, 2022 16:42
@sloretz
Copy link
Contributor Author

sloretz commented Mar 24, 2022

CI (build: --packages-above-and-dependencies rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp test: --packages-select rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp)

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

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