Add in REP-150: updated metapackages for ROS Melodic by clalancette · Pull Request #144 · ros-infrastructure/rep

clalancette · 2017-11-21T17:51:53Z

This is a REP meant to supplant REP-142, mainly to remove robot_model from the Melodic metapackages. This should essentially complete ros/robot_model#195.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

mikaelarguedas · 2017-11-21T18:13:08Z

This is a REP meant to supplant REP-142

Can you highlilght the differences compared to REP-142 to facilitate review

mainly to remove robot_model from the Melodic metapackages

Related to previous question: is this the only change in this REP or are there "non-main" changes that reviewer should be aware of ?

clalancette · 2017-11-21T18:18:25Z

Can you highlilght the differences compared to REP-142 to facilitate review

Sure thing. I ran a diff rep-0142.rst rep-0150.rst and uploaded the results here: https://gist.github.com/clalancette/2dc40937d2db2bd31d519562e83fe05b

Taking a closer look, the differences are in the metadata at the top, changing the wording from "Indigo" to "Melodic" throughout, adding a reference that this REP replaces 0142, some minor spelling fixes, and the actual change from using robot_model to using the split-out set of packages.

mikaelarguedas

One main comment to address is the fact that this PR does not remove the packages that motivated the spllit of robot_model in the first place.
The rest is mostly nitpicks

some minor spelling fixes,

Please consider opening a PR against REP-142 to fix these typos

mikaelarguedas · 2017-11-21T18:24:01Z

rep-0150.rst

+The `ros_core` metapackage composes the core communication protocols.
+It may not contain any GUI dependencies.
+In ROS Jade `ros_core` is extended to include `geneus`.
+In ROS Kinetic `ros_core` is extended to include `gennodejs`.


I don't think this belongs in this REP as this defines "Melodic and newer" metapackages

Yeah, I wasn't entirely sure here. I'll remove this language and only leave the things that reference "Melodic" (the same below).

mikaelarguedas · 2017-11-21T18:24:45Z

rep-0150.rst

+ - ros_core:
+      extends: [ros, ros_comm]
+      packages: [catkin, cmake_modules, common_msgs, console_bridge,
+                 gencpp, geneus(Jade and newer), gennodejs(Kinetic and newer),


Same here, this targets Melodic and newer, so when the generators have been added before Melodic isn't relevant IMO

mikaelarguedas · 2017-11-21T18:25:40Z

rep-0150.rst

+
+  - robot:
+      extends: [ros_base]
+      packages: [collada_parser, collada_urdf, control_msgs, diagnostics,


Looking at the original issue, the goal of splitting the robot_model repository was to remove collada_urdf and collada_parser completely from desktop_full. So I think they shouldn't be in this list.

mikaelarguedas · 2017-11-21T18:29:48Z

rep-0150.rst

+      packages: [common_tutorials, geometry_tutorials, ros_tutorials,
+                 roslint, visualization_tutorials]
+  - desktop_full:
+      extends: [desktop, perception, simulators, urdf_tutorials]


urdf_tutorials is not a variant defined in this document so it cannot be extended.
Maybe you meant the packages urdf_tutorial and urdf_tutorial_sim?

This is a copy of what is in REP-142; I didn't modify it. That being said, you are correct, and urdf_tutorials not only isn't in this document, it's not a package. Further, urdf_tutorial and urdf_tutorial_sim are both packages, but they aren't meta-packages, so they aren't really available to be extended anyway. Looking at https://github.com/ros/metapackages/blob/kinetic-devel/desktop_full/package.xml, it looks like urdf_tutorial is a dependency of desktop-full, but it is a package, so not for extension. I'm going to change this to put urdf_tutorials in a package section for desktop-full.

As per previous comment, I think that both urdf_tutorial_sim and urdf_tutorial should be added to desktop_full.
In lunar urdf_tutorial introduced a dependency on gazebo that forced us to move it from desktop to desktop_full. It has since been split into 2 packages urdf_tutorial and urdf_tutorial_sim to remove the dependency on the simulator from urdf_tutorial. As they are still located in the same repository they have to be released together, so we should put both in desktop_full IMO. See (ros/metapackages#13 and ros/urdf_tutorial#24 for context

mikaelarguedas · 2017-11-21T18:36:50Z

rep-0150.rst

@@ -0,0 +1,212 @@
+REP: 150
+Title: ROS Melodic and Newer Metapackages
+Author: Chris Lalancette <clalancette@openrobotics.org>


As this is mostly a copy-n-paste of an existing REP, my guess is that the original author should be mentioned (not sure how it works for REPs)?

Yeah, I'm not entirely sure how it works. Looking at REP-133, for instance, it has multiple authors, so I think I'll add multiple authors here.

mikaelarguedas · 2017-11-21T18:45:10Z

Sure thing. I ran a diff rep-0142.rst rep-0150.rst and uploaded the results here: https://gist.github.com/clalancette/2dc40937d2db2bd31d519562e83fe05b

Thanks for that, I didnt think there was so few differences :/ I should have ran the diff before asking...

clalancette · 2017-11-21T19:18:09Z

One main comment to address is the fact that this PR does not remove the packages that motivated the spllit of robot_model in the first place.

Right, good call. I'll definitely fix this (and the nitpicks).

clalancette · 2017-11-27T18:15:06Z

Please consider opening a PR against REP-142 to fix these typos

Done in #145

comments addressed

mikaelarguedas · 2017-11-27T18:47:16Z

rep-0150.rst

+The `desktop` variant is more minimal and only provides the stacks in
+the `robot` variant, plus visualization and debugging tools.
+Both of these variants contain tutorials for the stacks they provide.
+`urdf_tutorials` are only in `desktop_full` because they depend on the simulator.


urdf_tutorials << this is still not an existing package or variant. maybe use urdf_sim_tutorial here? or a non-quoted name like "the urdf tutorials are in desktop_full ..."

Yeah, good point. Will change to the latter without quotes.

mikaelarguedas · 2018-02-13T02:15:21Z

Please update this to highlight the differences from the previous REP as discussed in #144 (comment) and offline.

dirk-thomas · 2018-02-13T06:23:02Z

rep-0150.rst

@@ -0,0 +1,210 @@
+REP: 150
+Title: ROS Melodic and Newer Metapackages
+Author: Tully Foote <tfoote@willowgarage.com>, Chris Lalancette <clalancette@openrobotics.org>


Willow Garage email.

Yeah, I wasn't quite sure what to do about that. This file is largely a copy of REP-142, with some small differences, so I ended up keeping this email address the same. @tfoote , should I just change this to your @openrobotics.org address?

Yes please. The willowgarage address doesn't get delivered anymore.

Sure. Done in 7a5191e

clalancette · 2018-02-13T13:37:18Z

Please update this to highlight the differences from the previous REP as discussed in #144 (comment) and offline.

I think that is more-or-less done here: https://github.com/ros-infrastructure/rep/pull/144/files#diff-27a07c0481f895e29401386cc298c67dR95 . There are no other major changes to this document other than spelling fixes and metadata updates.

mikaelarguedas · 2018-02-13T15:54:00Z

I think that is more-or-less done here: https://github.com/ros-infrastructure/rep/pull/144/files#diff-27a07c0481f895e29401386cc298c67dR95 . There are no other major changes to this document other than spelling fixes and metadata updates.

Maybe it would be clearer if this was stated at the beginning of the REP (motivation section?) rather than halfway through ?

clalancette · 2018-02-13T16:04:02Z

Maybe it would be clearer if this was stated at the beginning of the REP (motivation section?) rather than halfway through ?

Sure, that makes sense. Done in b000ad1.

mikaelarguedas · 2018-02-14T17:22:39Z

The last commit makes CI fail. I think Tully's address needs to be updated in all REPs for CI to pass again

clalancette · 2018-02-14T18:32:30Z

The last commit makes CI fail. I think Tully's address needs to be updated in all REPs for CI to pass again

Does that test even make sense? Why can't authors have more than one email address?

mikaelarguedas · 2018-02-14T18:36:46Z

Does that test even make sense? Why can't authors have more than one email address?

I think it's mostly to make sure that people update all their email addresses and don't keep invalid email addresses in places they forgot to update. I'm not sure in what situation you would want emails for some REPs coming to one address and emails for other REPs sent to another email address...

tfoote · 2018-02-14T19:26:55Z

CI passing with my email updated everywhere.

clalancette · 2018-02-14T19:31:38Z

Great, thanks. If there are no further comments, I'll merge this by the end of the day.

mikaelarguedas · 2018-02-14T20:19:23Z

I'll merge this by the end of the day.

The urdf_tutorial vs urdf_tutorial_sim has not been finalized so this should be concluded before merging (as this will change what is in desktop vs desktop-full).

The repo has been created https://github.com/ros/urdf_sim_tutorial but the migration has not been done yet. @clalancette and @DLu have access to the repository to perform the change

clalancette · 2018-03-08T15:42:20Z

The repo has been created https://github.com/ros/urdf_sim_tutorial but the migration has not been done yet. @clalancette and @DLu have access to the repository to perform the change

All right, this migration is now done. We took the tactic of leaving the older distributions as they are, so we'll just have to release urdf_tutorial and urdf_sim_tutorial into Melodic from separate repositories. I also did a slight update to remove the sentence about urdf_*tutorial (since it is no longer special). With that, I think this is probably ready for another review and merging.

mikaelarguedas

finally found the time to do another pass on this, see comments below

mikaelarguedas · 2018-03-15T20:51:35Z

rep-0150.rst

+ - ros_core:
+      extends: [ros, ros_comm]
+      packages: [catkin, cmake_modules, common_msgs, console_bridge,
+                 gencpp, geneus, gennodejs, genlisp, genmsg, genpy,


Nit: this is out of alphabetical order

mikaelarguedas · 2018-03-15T20:53:12Z

rep-0150.rst

+
+ - ros_core:
+      extends: [ros, ros_comm]
+      packages: [catkin, cmake_modules, common_msgs, console_bridge,


console_bridge is not a released package anymore as it's available upstream Ubuntu (as libconsole-bridge-dev) since Trusty

mikaelarguedas · 2018-03-15T20:55:49Z

rep-0150.rst

+      packages: [catkin, cmake_modules, common_msgs, console_bridge,
+                 gencpp, geneus, gennodejs, genlisp, genmsg, genpy,
+                 message_generation, message_runtime, rosbag_migration_rule,
+                 rosconsole_bridge, roscpp_core, rosgraph_msgs, roslisp,


rosgraph_msgs is part of ros_comm so doesn't need to be listed here

rosgraph_msgs is part of ros_comm so doesn't need to be listed here

That is incorrect. rosgraph_msgs as well as std_srvs are not part of the ros_comm repo. Therefore they should be listed explicitly. Possible they could be combined into a single entry to be consistent with the rest: ros_comm_msgs.

I guess information I am missing here is: does extends refers to variants/packages or repositories?

mikaelarguedas · 2018-03-15T20:56:04Z

rep-0150.rst

+                 gencpp, geneus, gennodejs, genlisp, genmsg, genpy,
+                 message_generation, message_runtime, rosbag_migration_rule,
+                 rosconsole_bridge, roscpp_core, rosgraph_msgs, roslisp,
+                 rospack, std_msgs, std_srvs]


std_srvs is also part of ros_comm

mikaelarguedas · 2018-03-15T20:59:23Z

rep-0150.rst

+
+  - ros_base:
+      extends: [ros_core]
+      packages: [actionlib, angles, bond_core, class_loader,


while being part of the previous rep, angles is not part of ros_base on any distro according to the history of the metapackages

Not sure if the it's REP or the metapackage that should be modified. Maybe @tfoote or @dirk-thomas have some feedback?

I'd keep angles out of the ros_base. It's certainly low level, but it's really a library that is a build and exec dependency with no user entry points so it will be brought in by any dependencies necessary but isn't actually useful on its own.

angles should not just be removed. It needs to be moved to the variant which it is actually included in instead. Since it is a dependency of turtle_actionlib which is part of common_tutorials it should go into the same variant.

mikaelarguedas · 2018-03-15T21:04:46Z

rep-0150.rst

+
+  - robot:
+      extends: [ros_base]
+      packages: [collada_parser, control_msgs, diagnostics, executive_smach,


collada_urdf has been removed from that list but not collada_parser. According to this the original review we should have removed both

mikaelarguedas · 2018-03-15T21:07:18Z

rep-0150.rst

+                 stage_ros]
+
+  - viz:
+      extends: [robot]


the metapackage viz currently extends ros_base and not robot

same here, not sure if the it's REP or the metapackage that should be modified. Maybe @tfoote or @dirk-thomas have some feedback?

The robot target was to think what you'd want on the robot, whereas viz is for the developer interface so I think that it makes sense to stick with the ros_base.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

1. Add Tully back as one of the authors. 2. Remove collada_urdf from the packages for robot. 3. Remove all language prior to Melodic. 4. Fix the desktop_full variant to make urdf_tutorial a package. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas · 2018-03-16T18:33:41Z

Can you please post a diff how the variants have changed between the previous REP and this version to ease the review.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-03-16T18:41:57Z

Can you please post a diff how the variants have changed between the previous REP and this version to ease the review.

Full diff is here (diff -u rep-0142.rst rep-0150.rst): https://gist.github.com/clalancette/2dc40937d2db2bd31d519562e83fe05b

Summary is:

Remove console_bridge, rosgraph_msgs, std_srvs from ros_core
Remove angles from ros_base
Remove robot_model, add joint_state_publisher, kdl_parser, kdl_parser_py, urdf, urdf_parser_plugin to robot
Change viz to extend from ros_base
Add urdf_tutorial to desktop
Remove urdf_tutorial, add urdf_sim_tutorial to desktop_full

dirk-thomas · 2018-03-16T18:48:49Z

Summary is:

Please elaborate on each of these change why they are being made. Just looking at the first bullet they seem incorrect since these are even dependencies of the other packages in ros_core.

clalancette · 2018-03-16T18:53:48Z

Please elaborate on each of these change why they are being made. Just looking at the first bullet they seem incorrect since these are even dependencies of the other packages in ros_core.

The original goal of this PR was to update the metapackages for Melodic, specifically to:

Remove robot_model and split it into individual packages (kdl_parser, urdf, etc), and
Split urdf_tutorials into urdf_tutorial and urdf_sim_tutorial so that we can have the former in desktop and the latter in desktop_full.

The other changes came during review; @mikaelarguedas gave some feedback on why he thought those should be made in the reviews above.

dirk-thomas · 2018-03-16T19:12:04Z

rep-0150.rst

+::
+
+ - ros_core:
+      extends: [ros, ros_comm]


does extends refers to variants/packages or repositories?

This line doesn't make any sense and needs to be removed. Probably from REP 142 too.

So the answer is "it applies to packages/variants and not repositories" ?

What you are suggesting is to remove that line and to add ros_comm and ros to the list of packages? is that correct?

So the answer is "it applies to packages/variants and not repositories" ?

All other variants list variant names in the extends.

What you are suggesting is to remove that line and to add ros_comm and ros to the list of packages? is that correct?

Yes.

So now that ros and ros_comm are listed as packages (and not repositories). Do we need to keep std_srvs and rosgraph_msgs listed given that they are dependencies of ros_comm?

Do we need to keep std_srvs and rosgraph_msgs listed given that they are dependencies of ros_comm?

With the same argument you could remove almost everything from the list, e.g. catkin since everything else depends on it. The goal is to have a complete list which answers the question what is in a variant. This needs to be obvious without starting a dependency traversal for packages / repos which are not listed here explicitly.

Ok so the idea is that we should list all packages in a variant, and if there is a metapackage in their source repository, there is no need to list the dependencies of that metapackage, as long as they are hosted in the same repository.
Is that a good summary?

I would say the list enumerates ROS packages / repositories.

1. Make ros and ros_comm packages as part of ros_core 2. Restore rosgraph_msgs and std_srvs as part of ros_core 3. Add angles to desktop Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-03-16T21:37:14Z

The latest commit should address @dirk-thomas comments. Now the differences between REP-142 and REP-150 are:

Remove "extends" from ros_core completely, since it doesn't make sense (this needs to go back into REP-142)
Add ros and ros_comm, remove console_bridge from ros_core package list (this needs to go back into REP-142)
Remove angles from ros_base
Remove robot_model, add joint_state_publisher, kdl_parser, kdl_parser_py, urdf, urdf_parser_plugin to robot
Change viz to extend from ros_base
Add angles and urdf_tutorial to desktop
Remove urdf_tutorial from extends, add urdf_sim_tutorial to packages for desktop_full

Does this look better now?

dirk-thomas · 2018-03-16T21:39:32Z

Add ros and ros_comm, remove console_bridge from ros_core package list (this needs to go back into REP-142)

Why should the console_bridge change be backported to REP 142?

mikaelarguedas · 2018-03-16T21:47:42Z

Change viz to extend from ros_base

this should be backported to REP-142 as well

Add angles and urdf_tutorial to desktop

the move of angles from ros_base to desktop should be backported to REP-142 as well

mikaelarguedas · 2018-03-16T22:38:15Z

Why should the console_bridge change be backported to REP 142?

Because there is no package called console_bridge in Indigo and later (we use upstream #144 (comment))

dirk-thomas · 2018-03-16T22:41:45Z

Because there is no package called console_bridge in Indigo and later

I thought we used upstream only later. Sounds good then.

mikaelarguedas

lgtm, with follow-up PR on REP-142

clalancette · 2018-03-19T13:20:18Z

Thanks for the reviews. I've now opened #165 to deal with the follow-up for REP-142. I'm going to merge this now.

sloretz mentioned this pull request Nov 21, 2017

Split this repo and deprecate robot_model ros/robot_model#195

Closed

9 tasks

mikaelarguedas previously requested changes Nov 21, 2017

View reviewed changes

mikaelarguedas reviewed Nov 27, 2017

View reviewed changes

clalancette mentioned this pull request Nov 27, 2017

dependency on gazebo_ros ros/urdf_tutorial#24

Closed

sloretz approved these changes Feb 13, 2018

View reviewed changes

dirk-thomas reviewed Feb 13, 2018

View reviewed changes

clalancette force-pushed the rep150 branch from ed3e42d to b000ad1 Compare February 13, 2018 16:03

tfoote force-pushed the rep150 branch from cc5f7e7 to 0116e9e Compare February 14, 2018 18:53

mikaelarguedas changed the title ~~Add in REP-150~~ Add in REP-150: updated metapackages for ROS Melodic Mar 15, 2018

mikaelarguedas reviewed Mar 15, 2018

View reviewed changes

clalancette added 3 commits March 16, 2018 13:46

Add in REP-150

51de44b

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Fixes from review.

a010e0f

1. Add Tully back as one of the authors. 2. Remove collada_urdf from the packages for robot. 3. Remove all language prior to Melodic. 4. Fix the desktop_full variant to make urdf_tutorial a package. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add urdf_sim_tutorial to the list of packages in desktop_full.

9545600

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette and others added 7 commits March 16, 2018 13:46

Fix up the wording around urdf tutorials.

3f3de72

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Move the changes up to the Motivation section.

b241ce9

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Fix Tully's email address.

0e2d71c

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Update Tully's email address everwhere.

25a5a9f

Move urdf_tutorial into desktop.

e2cbf2e

Remove an obsolete comment.

759465e

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

More fixes from review.

d7fe9d9

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette force-pushed the rep150 branch from 9a58738 to d7fe9d9 Compare March 16, 2018 17:53

Remove duplicate rosconsole_bridge.

fb7f3f7

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas reviewed Mar 16, 2018

View reviewed changes

More fixes from review.

51e2eaf

1. Make ros and ros_comm packages as part of ros_core 2. Restore rosgraph_msgs and std_srvs as part of ros_core 3. Add angles to desktop Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas approved these changes Mar 16, 2018

View reviewed changes

mikaelarguedas approved these changes Mar 16, 2018

View reviewed changes

mikaelarguedas mentioned this pull request Mar 16, 2018

update metapackages to match variant definitions of REP-150 ros/metapackages#17

Merged

clalancette merged commit 6d7ef4a into master Mar 19, 2018

clalancette deleted the rep150 branch March 19, 2018 13:20

mikaelarguedas mentioned this pull request Sep 12, 2018

REP-2001 ROS 2 Variants initial draft #176

Merged

matthew-reynolds mentioned this pull request Dec 18, 2019

joint_state_publisher in metapackage robot depends on Qt ros/metapackages#28

Closed

Conversation

clalancette commented Nov 21, 2017

Uh oh!

mikaelarguedas commented Nov 21, 2017

Uh oh!

clalancette commented Nov 21, 2017

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Nov 21, 2017

Uh oh!

clalancette commented Nov 21, 2017

Uh oh!

clalancette commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette commented Feb 13, 2018

Uh oh!

mikaelarguedas commented Feb 13, 2018

Uh oh!

clalancette commented Feb 13, 2018

Uh oh!

mikaelarguedas commented Feb 14, 2018

Uh oh!

clalancette commented Feb 14, 2018

Uh oh!

mikaelarguedas commented Feb 14, 2018

Uh oh!

tfoote commented Feb 14, 2018

Uh oh!

clalancette commented Feb 14, 2018

Uh oh!

mikaelarguedas commented Feb 14, 2018

Uh oh!

clalancette commented Mar 8, 2018

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment