search

ros2 / rosidl

Packages which provide the ROS IDL (.msg) definition and code generation.
Apache License 2.0
78 stars 125 forks source link

Splitted rosidl_generator_c and rosidl_generator_cpp in two: rosidl_generator_x and rosidl_runtime_x #443

Closed ahcorde closed 4 years ago

ahcorde commented 4 years ago

This issue is to track all the PRs that are need it to compile ros2.repos with the changes introduced in this PR split https://github.com/ros2/rosidl/pull/442. We will have these packages:

ahcorde commented 4 years ago
dirk-thomas commented 4 years ago

It would make it easier to review these PRs if you could reference at least one specific source line in the packages where new dependencies are added to clarify why they are added. It might also be good to clarify - when runtime packages are added - why generators are still needed.

ahcorde commented 4 years ago

It might also be good to clarify - when runtime packages are added - why generators are still needed.

This was my fault I didn't remove the rosidl_generator_c/cpp dependencies. I revisited all the changes and removed this dependencies

ahcorde commented 4 years ago

I have been checking rcl, rcl_lifecycle, rcl_action, rclcpp and rclcpp_action all of them make use of rosidl_generator ( with the new packages should be rosidl_runtime) but I think this dependecy is not need it, the reason why it's because rosidl_generator/rosidl_runtim is used where a message packages it's being used too.

Some examples: RCL: https://github.com/ros2/rcl/blob/master/rcl_action/CMakeLists.txt#L57 https://github.com/ros2/rcl/blob/master/rcl_lifecycle/CMakeLists.txt#L48

RCLCPP:

https://github.com/ros2/rclcpp/blob/master/rclcpp_action/CMakeLists.txt#L36 https://github.com/ros2/rclcpp/blob/master/rclcpp/CMakeLists.txt#L180

@wjwwood and @dirk-thomas what do you think?

ahcorde commented 4 years ago

testing all PRs

ahcorde commented 4 years ago
ahcorde commented 4 years ago
dirk-thomas commented 4 years ago

Please double check that with all the PRs the actual goal (avoiding a runtime dependency on any generator package) has been achieved.

ahcorde commented 4 years ago

rmw_connext and rosidl_typesupport_connext approved. Launching CI for all the commits:

ahcorde commented 4 years ago

I have been reproducing the failure in my local MAC OS machine. if I used include_directories there is no issue when cmake is trying to find the generated headers, but with target_include_directories, I think a race condition is happening. If I execute colcon a second time then the error disappears.

@wjwwood or @dirk any thoughts about this?

dirk-thomas commented 4 years ago

The rosidl_generator_cpp PR removes the add_dependencies(<test_target> ${PROJECT_NAME}) calls for all test targets: e.g. https://github.com/ros2/rosidl/pull/442/files#diff-59c331d207ef4d5e6a390e74e0bc4739L54

Without that dependency CMake tries to build the test before the code generator was run which creates the used header files.

ahcorde commented 4 years ago
ahcorde commented 4 years ago

The packages depend on rosidl_generator_c or rosidl_generator_cpp are:

The packages depend on rosidl_runtime_c or rosidl_runtime_cpp

dirk-thomas commented 4 years ago

The packages depend on rosidl_generator_c or rosidl_generator_cpp are:

I guess the dependencies still need to be removed from the two introspection packages then to achieve the stated goal, right?

ahcorde commented 4 years ago

The packages depend on rosidl_generator_c or rosidl_generator_cpp are:

The packages depend on rosidl_runtime_c or rosidl_runtime_cpp

ahcorde commented 4 years ago
ahcorde commented 4 years ago

The packages depend on rosidl_generator_c or rosidl_generator_cpp are:

The packages depend on rosidl_runtime_c or rosidl_runtime_cpp

dirk-thomas commented 4 years ago

Please update all PRs to update / revert changes to dependency types of the following kind, e.g. ros2/rmw_dds_common#10:

Since the added buildtool_depend and buildtool_export_depend dependencies are a build as well as a run dependency please revert the dependency type to the previously used depend. I would rather like to avoid that kind of dependency type changes in this already massive amount of changes.

dirk-thomas commented 4 years ago

Also please update the list of all related PRs somewhere in this ticket - in a single place (rather than having to scroll through the whole thread).

ahcorde commented 4 years ago

The packages depend on rosidl_generator_c or rosidl_generator_cpp are:

The packages depend on rosidl_runtime_c or rosidl_runtime_cpp

update / revert changes to dependency types

ahcorde commented 4 years ago
ahcorde commented 4 years ago
ahcorde commented 4 years ago
ahcorde commented 4 years ago

These warnings on Windows were already fixed, but I didn't update the branch in the PR

15:49:21 C:\ci\ws\src\ros2\rmw_dds_common\rmw_dds_common\test\test_graph_cache.cpp(195,8): warning C4996: 'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\rmw_dds_common\test_graph_cache.vcxproj]

15:49:21 C:\ci\ws\src\ros2\rmw_dds_common\rmw_dds_common\test\test_graph_cache.cpp(388,8): warning C4996: 'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\rmw_dds_common\test_graph_cache.vcxproj]
ahcorde commented 4 years ago

Testing changes on rmw_cyclonedss

ahcorde commented 4 years ago
ahcorde commented 4 years ago
ahcorde commented 4 years ago
ahcorde commented 4 years ago 
CMake Deprecation Warning at /home/jenkins-agent/workspace/ci_linux-aarch64/ws/install/ament_cmake_export_interfaces/share/ament_cmake_export_interfaces/cmake/ament_export_interfaces.cmake:37 (message):
ament_export_interfaces() is deprecated, use ament_export_targets() instead
Call Stack (most recent call first):
CMakeLists.txt:85 (ament_export_interfaces)

In MacOS

Windows