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.

Sign up for GitHub

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

Jump to bottom

Removed tornado source code, added dependency #317

Merged
merged 1 commit into from
Feb 1, 2018
Merged

Removed tornado source code, added dependency #317

merged 1 commit into from
Feb 1, 2018

Conversation

MBlistein
Copy link
Contributor

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
Copy link
Contributor

T045T 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
jihoonl requested changes Jan 15, 2018
View reviewed changes
Copy link
Member

@jihoonl jihoonl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

rosbridge_server/package.xml Outdated
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remove rosauth. It is in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MBlistein
Copy link
Contributor Author

MBlistein commented Jan 23, 2018 •
edited
Loading

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.

@Behery Behery self-requested a review January 23, 2018 14:08
Behery
Behery previously requested changes Jan 23, 2018
View reviewed changes
Copy link
Member

@Behery Behery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@jihoonl
Copy link
Member

jihoonl 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
Copy link
Member

jihoonl commented Jan 26, 2018

Let me ping ros maintainer to have a last check.

@Behery Behery dismissed their stale review January 26, 2018 19:29

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

@MBlistein
Copy link
Contributor Author

Ok, thanks! I did rebase in the meantime.

@jihoonl jihoonl merged commit 791261c into RobotWebTools:develop Feb 1, 2018
@jihoonl jihoonl mentioned this pull request Apr 9, 2018
Merged
@jihoonl
Copy link
Member

jihoonl 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
Copy link
Contributor Author

@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 removed_tornado branch May 4, 2018 13:44
@jihoonl jihoonl mentioned this pull request Jul 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jihoonl jihoonl jihoonl approved these changes

@Behery Behery Behery left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MBlistein @T045T @jihoonl @Behery @M-Blistein