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

Removed tornado source code, added dependency #317

Merged
merged 1 commit into from Feb 1, 2018

Conversation

Projects
None yet
5 participants
@MBlistein
Copy link
Contributor

commented Jan 11, 2018

There are debian packages for both tornado and backports, that contain newer versions as the source code copied inside of rosbridge_server.
Therefore the source code has been deleted and replaced with run dependencies on the corresponding debian packages.

@T045T

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2018

There have been attempts to switch to an external version before (see #162 for an overview and links to related issues).

I'm not sure if there aren't any subtle issues with using an external tornado.
@rctoris , @jihoonl , @jonbinney: Looks like you were involved in the previous effort, do you know of any showstoppers here?

@jihoonl
Copy link
Member

left a comment

This is necessary change for the future releases. It was kept for legacy and backward compatibility reason. We can probably drop tornado and backport from lunar easily.

It becomes problematic for kinetic. I am not sure how it would affect the current installation if user do apt upgrade or apt dist-upgrade. @MBlistein can you try to install this as a deb? I would like to answer the following questions.

  • What happen to the current installed version?
  • how does it change /opt/ros/kinetic/lib/python2.7? Does it remove tornado and backport?
  • Does rosbridge work fine with the new version of tornado and backport?

Maybe.. @jspricke could you help @MBlistein to test rosbridge server debian installation?

<run_depend>python-twisted-core</run_depend>
<run_depend>rosbridge_library</run_depend>
<run_depend>rosapi</run_depend>
<run_depend>rospy</run_depend>
<run_depend>rosauth</run_depend>

This comment has been minimized.

Copy link
@jihoonl

jihoonl Jan 15, 2018

Member

don't remove rosauth. It is in the code

This comment has been minimized.

Copy link
@MBlistein

MBlistein Jan 16, 2018

Author Contributor

Ok, @jspricke will create a debian package and we will look at testing together with @Behery next week.

@MBlistein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

Hey,
big thanks to jspricke who made a debian package and Behery who tested it. It seems to work out:

  • when installing / upgrading, it overwrites the existing installation in opt/ros/kinetic
  • tornado and backport are uninstalled (but corresponding debian packages installed via apt)
  • the rosbridge server is functional:
  • subscribing/ unsubscribing/ publishing to topics works
  • service calls go through, advertising a service also works

Should be good to go, I will update the pull request with your remark in a minute.

@MBlistein MBlistein force-pushed the magazino:removed_tornado branch from b407f38 to 38089e8 Jan 23, 2018

@Behery Behery self-requested a review Jan 23, 2018

@Behery
Copy link
Member

left a comment

@MBlistein can you rebase your branch on top of develop instead of merge?
After that I think we can merge, @jihoonl what do you think?

@MBlistein MBlistein force-pushed the magazino:removed_tornado branch from c4e981d to b7f7d9b Jan 24, 2018

@jihoonl

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Since we plan to squash and merge, it should be fine to have a merge commit. It will get squashed as long as there is no conflict.

@jihoonl

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Let me ping ros maintainer to have a last check.

@Behery Behery dismissed their stale review Jan 26, 2018

I'm removing my review since it only had to do with the rebase vs merge commits

@MBlistein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

Ok, thanks! I did rebase in the meantime.

@jihoonl

jihoonl approved these changes Feb 1, 2018

@jihoonl jihoonl merged commit 791261c into RobotWebTools:develop Feb 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jihoonl

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@MBlistein hey I just released the newer version of rosbridge. You may want to test ros-shadow-fixed for final checking.

@MBlistein

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

@jihoonl Sorry for the late reply. Tested it (thanks to @Behery ) and it seems fine. Luckily, since already released out of shadow-fixed :)

@MBlistein MBlistein deleted the magazino:removed_tornado branch May 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.