Split jsp and jsp gui

[clalancette]

This PR splits joint_state_publisher into two packages: joint_state_publisher and joint_state_publisher_gui. The joint_state_publisher package contains both code to be imported (the JointStatePublisher class) along with a script that just runs the basic, non-GUI functionality of JointStatePublisher. The joint_state_publisher_gui package contains the PyQt UI that allows one to control the movable joint via a UI, and depends on the joint_state_publisher package and JointStatePublisher class.

Doing this is desirable for a few reasons, mainly to slim down joint_state_publisher and remove the Qt dependencies from the robot metapackage (see https://github.com/ros/metapackages/issues/28 for details).

I am unsure where to target this PR. This is definitely a breaking change in that users who just install/launch joint_state_publisher will no longer have GUI functionality available. So I would usually target noetic with this. However, I think that removing the GUI dependencies from the robot metapackage in kinetic and melodic is important, so I'm currently targeting this at kinetic-devel. I'm definitely open to discussing this further.

This also fixes #21 .

[clalancette]

I think you're right to target Kinetic & Melodic, though I think we'd be better off with a smoother migration as described in my comment. Then for Noetic we can drop the forwarding altogether. That is, rather than failing outright, attempt to minimize the breaking change by searching for and launching joint_state_publisher_gui if possible, with a warning telling users about the change. In the case that the joint_state_publisher_gui package isn't found, print an error and exit(1) as you've done here.

Stuck between a rock and a hard place here. If we do this, then by all rights joint_state_publisher should have a dependency on joint_state_publisher_gui. Since joint_state_publisher_gui already has a dependency on joint_state_publisher, we end up with a circular dependency. On the other hand, if we don't do this, then we end up with potentially surprising results as you point out. I guess if we have a graceful fallback option, we can just live with the incorrect dependency hierarchy. I can take a look at doing that after the holidays.

Couple other comments:

Since you're restructuring and creating a bunch of new files, is this a good opportunity to add license header comments? It appears the other former robot_model packages all have the expected 3-clause BSD license w/ Willow Garage as the copyright holder.

Good point. I've added the license stuff in 1db983d.

Don't think the screenshot.png needs to be copied over to the new package.

Actually, I think it is the other way around; joint_state_publisher itself no longer needs it (since it is a screenshot of the GUI). So I've moved the screenshot over to the GUI package.

Thanks for the review.

[matthew-reynolds]

I guess if we have a graceful fallback option, we can just live with the incorrect dependency hierarchy

Yeah that's pretty much what I'm suggesting. Rather than an exec_depend on joint_state_publisher_gui, we have a sort of "soft dependency" where we instruct the user to install the package. Definitely not a clean dependency hierarchy, but it smooths out the migration. Especially since I suspect the "soft dependency" will resolve for most users, since if they're using the GUI they're likely on a desktop metapackage install anyways.

Actually, I think it is the other way around

Good call 👍

[clalancette]

All right, I've now updated it so that joint_state_publisher has a "soft" dependency on joint_state_publisher_gui, and will attempt to launch the latter if use_gui is set to True. This mimics backwards-compatible behavior with what exists in Kinetic and Melodic. Assuming we merge this PR, I intend to make a new noetic branch where we remove this workaround.

@sloretz @matthew-reynolds Ready for another round of review.

[clalancette]

@sloretz Thanks for the partial review, I've fixed the issues you've identified. Ready for complete review/second round of review.

[clalancette]

Thanks! Merging now that the buildfarm is turning over.

[matthew-reynolds]

@clalancette @sloretz Do you plan on updating REP 142/150 to add joint_state_publisher_gui to the desktop metapackage? Happy to provide a PR if this is your plan.

[clalancette]

@clalancette @sloretz Do you plan on updating REP 142/150 to add joint_state_publisher to the desktop metapackage? Happy to provide a PR if this is your plan.

As far as I can tell, REP-142 doesn't need to be updated. It doesn't include joint_state_publisher in the robot metapackage, and I don't think we should change that this late in its lifetime.

REP-150 is obviously out-of-sync with what is in https://github.com/ros/metapackages; I'll submit a PR to update that soon.

Arg, never mind, I was confused. I forgot about the soft dependency thing.

So it is still the case that REP-142 doesn't have joint_state_publisher at all, so I don't think we need to do anything there. REP-150 should indeed be updated to add joint_state_publisher_gui to the desktop metapackage; I'll open a PR for that soon.

[clalancette]

https://github.com/ros/metapackages/pull/30 and https://github.com/ros-infrastructure/rep/pull/230

[matthew-reynolds]

REP-142 doesn't have joint_state_publisher at all, so I don't think we need to do anything there

jsp is pulled in via robot_model (in the robot metapackage) and urdf_tutorial (in the desktop_full metapackage). Seems risky to not add jsp_gui to desktop since previously every install from robot upwards contained the gui.

REP-150 should indeed be updated to add joint_state_publisher_gui to the desktop metapackage

:+1:

[matthew-reynolds]

I suppose we should also give a scan through the dependant packages on jsp and update them accordingly (ie. urdf_tutorial uses the jsp gui, but depends only on jsp). I will get started on this.

[gavanderhoorn]

We're getting some questions on ROS Answers about this split (344992 fi).

Should this change be announced on Discourse to reduce the surprise?

[clalancette]

@gavanderhoorn Thanks for fielding those ROS answers queries.

I'll go ahead and make an announcement on Discourse.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/split-of-joint-state-publisher-and-joint-state-publisher-gui/12997/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/15