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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ros2] Split conversions into headers specific to message packages #803

Merged
merged 9 commits into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@dhood
Copy link

commented Aug 22, 2018

Otherwise any package that included conversions.hpp needed to build depend on all of the message packages included in that header.

Noticed the need for this because since #785 has been merged, gazebo_ros on the ros2 branch can't be built in isolation (missing dependency on sensor_msgs):

In file included from /home/dhood/ros2_ws/src/ros2/gazebo_ros_pkgs/gazebo_ros/test/test_conversions.cpp:15:0:
/home/dhood/ros2_ws/src/ros2/gazebo_ros_pkgs/gazebo_ros/include/gazebo_ros/conversions.hpp:30:44: fatal error: sensor_msgs/msg/point_cloud2.hpp: No such file or directory

A similar thing happened when #784 was merged requiring geometry_msgs (fixed in 80f3a78).

@chapulina not sure how you have been building locally but since the buildfarm won't catch issues building in isolation, if you can switch to an isolated build that will help catch these issues earlier 馃槃 (colcon build will use isolation by default)

@dhood dhood added the ros2 label Aug 22, 2018

@dhood dhood self-assigned this Aug 22, 2018

@dhood dhood requested a review from tfoote Aug 22, 2018

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

humm strange, I've always just used plain colcon build locally and can't reproduce the issue 馃 But it sounds like the fix is important anyway, thanks for catching that.

Regarding the separation of files, this is something I agree is a problem and the issue had already crossed my mind. The only issue I see with the current split is that it only considers one side of the conversion, which seems to be always a ROS message. But each conversion file will still potentially need to include various Gazebo, Ignition and SDF headers.

So I suggest we split according to pairs. For example, tf2 conversions are separated into packages which convert from tf2 to something else. So we'd get headers like these:

  • sensor_msgs-ign_math
  • sensor_msgs-gz_msgs
  • sensor_msgs-ign_msgs
  • sensor_msgs-gz_common
  • etc.

Does this sound too fine-grained?

@tfoote
Copy link
Member

left a comment

As we mentioned in the meeting this looks good with the addition of the necessary build_export_depend http://www.ros.org/reps/rep-0140.html#build-export-depend-multiple tags

@chapulina

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Conclusions from the meeting:

  • This is an improvement for package dependencies, let's go with the current split for now
  • In the future we may split further not to bring unnecessary includes
  • In the future, TF2's conversion functions could be promoted to a more common package such as rcl-utils so that they can be used for non-tf conversions as well
@dhood

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

@tfoote does this look alright now after 9803d5c? gazebo_ros has the conversions, and so exports a dependency on the three message packages for which conversions are supported. It build depends on builtin_interfaces because gazebo_ros_init uses it, and it additionally test depends on the other two message packages because test_conversions uses them.

@tfoote

tfoote approved these changes Aug 24, 2018

Copy link
Member

left a comment

This looks good to me. I had to update one of the depends. Added in 45137fb before this I couldn't compile in my fresh workspace.


<exec_depend>rclcpp</exec_depend>
<exec_depend>gazebo_dev</exec_depend>
<build_export_depend>builtin_interfaces</build_export_depend>

This comment has been minimized.

Copy link
@tfoote

tfoote Aug 24, 2018

Member

Adding this is redundant with changing the above build_depend to depend

- The generic dependency on 'builtin_interfaces' is redundant with: build_export_depend
@dhood

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

thanks!

@tfoote tfoote changed the title Split conversions into headers specific to message packages [ros2] Split conversions into headers specific to message packages Aug 26, 2018

@chapulina chapulina merged commit 6fc853a into ros2 Aug 28, 2018

2 checks passed

Bpr__gazebo_ros_pkgs__ubuntu_bionic_amd64 Build finished.
Details
ros2_gazebo_pkgs-ci-pr_any_bouncy-bionic-amd64 Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.