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

http_relay: 1.0.1-1 in 'noetic/distribution.yaml' [bloom] #37717

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Jun 19, 2023

Increasing version of package(s) in repository http_relay to 1.0.1-1:

http_relay

* Noetic compatibility.
* Added sigkill_on_stream_stop so that the relay is only killed if there is a stale request.
* Added sigkill_timeout for automatic restarting of the node even if it hangs on a connection.
* Support NTRIP in Python 3
* Fix Python3 NTRIP relay.
* Initial commit.
* Contributors: Martin Pecka

@github-actions github-actions bot added the noetic Issue/PR is for the ROS 1 Noetic distribution label Jun 19, 2023
@clalancette
Copy link
Contributor

See my comments in #37716 (review) about this package.

@adityapande-1995 adityapande-1995 added the changes requested Maintainers have asked for changes to the pull request label Jun 20, 2023
@clalancette
Copy link
Contributor

Reproducing the conversation from #37716

clalancette

This is another name that I think is pretty generic, and I'm actually not sure at all why it needs to be a ROS package.
That is, it really just reverse proxies HTTP requests, and the only thing it seems to use ROS for is to get parameters from 
the global parameter server. It seems to me that this functionality could just be satisfied by pip installing a Python package.

That said, I could be missing something about how it operates, so feel free to correct me.

peci1:

I agree this functionality isn't directly ROS-related. But it is robot-related.
A few times we had devices which could not access outer networks directly.

I searched the whole apt and pip, but i haven't found any simple package for http proxying (except for big beefy frameworks).

And as it wasn't the first time we needed to run a http relay on the robots, I felt this would be a good package candidate.

Our latest use-case is directly embedding the proxy into sensor ROS driver.

I think this package follows the naming REP. The name clearly says what it does.
Name collisions don't matter because any other package trying to achieve similar functionality would basically do the same.
And org prefix doesn't make sense because there is nothing org-specific there.

@clalancette
Copy link
Contributor

I searched the whole apt and pip, but i haven't found any simple package for http proxying (except for big beefy frameworks).

That is very surprising, but OK.

And as it wasn't the first time we needed to run a http relay on the robots, I felt this would be a good package candidate.

I think my big issue with adding this is that we aren't aiming to be a generic Linux distribution like Debian or Ubuntu. ROS distributions are more focused on robotics than that. While I agree that doing http proxying is sometimes useful and necessary in robotics, there is nothing robot-specific about it. There should be packages from the wider ecosystem that can do this for you. If something like this doesn't exist in PyPI.org, for instance, then I would suggest that this would be a contribution there.

@sloretz
Copy link
Contributor

sloretz commented Jul 11, 2023

If something like this doesn't exist in PyPI.org, for instance, then I would suggest that this would be a contribution there.

If they later want to release a package that depends on this code (say a *_bringup package containing launch files to launch everything on a robot), then it will be required to be apt-installable.

@peci1
Copy link
Contributor Author

peci1 commented Jul 12, 2023

If they later want to release a package that depends on this code (say a *_bringup package containing launch files to launch everything on a robot), then it will be required to be apt-installable.

Exactly. I agree it would make sense to try to push such a package into Ubuntu repos, but that is a much more far-fetched way then getting it via ROS buildfarm. So this ROS package is a kind of shortcut. But there's no way of getting the package to Ubuntu repos for Focal, so the ROS way is actually the only possible currently.

@emersonknapp
Copy link
Contributor

I agree it would make sense to try to push such a package into Ubuntu repos, but that is a much more far-fetched way then getting it via ROS buildfarm

I might suggest reading through https://wiki.debian.org/DebianMentorsFaq#Packaging - especially "How do I add a new package to the archive?". I don't think the process is so very long, and this would expand your reach beyond ROS distributions.

this ROS package is a kind of shortcut

The ROS buildfarm is not "free" in the sense of requiring no resources to maintain - so I'd personally prefer if we avoided using it as a shortcut to properly releasing generically useful software. Anybody who wants the package may check it out as source, so they are not blocked from using it - and PyPI has no barrier to entry at all.

@nuclearsandwich
Copy link
Member

I am usually the first person to encourage pushing dependencies further upstream and maintaining a tight focus in the ROS distributions on packages that either rely on ROS packages or are quite exclusively dependencies of ROS packages with little other utility.

I don't disagree with any of the arguments toward pushing this functionality upstream. There are a couple of points of nuance I want to recognize:

Putting packages in PyPI is of extremely limited utility if your ultimate goal is depending on it in an officially released ROS package

The most immediate value I see for releasing a Python package to PyPI is that PyPI is the closest thing to a global registry for Python module names, thus being present in that registry makes it less likely to collide with another python package when submitting your package for inclusion in Debian/Ubuntu and Fedora/EPEL.

For dependencies which are already on the path to being upstreamed, I think that these can be considered for vendoring in order to start building on them today. If there's a package that's in Debian Unstable or even an Intent-To-Package bug filed for it, and the package itself is not something that is already available on our target platforms but at a different version. I would consider allowing that to be included in a ROS distribution with the expectation that it will eventually be removed in favor of the upstream package.

@sloretz sloretz added the held for sync Issue/PR has been held because the distribution is in a sync hold label Aug 1, 2023
@sloretz
Copy link
Contributor

sloretz commented Aug 1, 2023

Holding for Noetic sync

@130s
Copy link
Member

130s commented Aug 5, 2023

In this thread there's some words with wisdom that I want to see spread across the communities. I'm also afraid this sort of discussion keeps happening (may not be many times though).
If no one cares to, I'm tempted to challenge myself to document it. Where can be a good place for such an info? Under How To Guide? Under Tutorials/Advanced?

@peci1
Copy link
Contributor Author

peci1 commented Aug 16, 2023

For dependencies which are already on the path to being upstreamed, I think that these can be considered for vendoring in order to start building on them today. If there's a package that's in Debian Unstable or even an Intent-To-Package bug filed for it, and the package itself is not something that is already available on our target platforms but at a different version. I would consider allowing that to be included in a ROS distribution with the expectation that it will eventually be removed in favor of the upstream package.

Thanks, I decided to go that way. I created pip package https://pypi.org/project/http-relay/ and now I'll try to make the ITP. I'll ping here when that's done to decide what to do with this proposed ROS package.

@peci1
Copy link
Contributor Author

peci1 commented Aug 18, 2023

The ITP is finished! https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050035 . I uploaded the built ubuntu packages to https://launchpad.net/~peci1/+archive/ubuntu/http-relay . Do you know of a Debian sponsor who might be interested?

@mjcarroll
Copy link
Member

@peci1, @j-rivero has offered to help connect you with a Debian sponsor.

@peci1
Copy link
Contributor Author

peci1 commented Aug 29, 2023

Great, thanks! Should I contact him or will he contact me?

@sloretz
Copy link
Contributor

sloretz commented Aug 30, 2023

Holding for Noetic sync

@j-rivero
Copy link
Contributor

Great, thanks! Should I contact him or will he contact me?

Could you please mail me (jrivero@osrf..) with the repository where you have the debian/ metadata? Thanks.

@sloretz sloretz removed the held for sync Issue/PR has been held because the distribution is in a sync hold label Sep 1, 2023
@sloretz sloretz added the held for sync Issue/PR has been held because the distribution is in a sync hold label Sep 12, 2023
@sloretz
Copy link
Contributor

sloretz commented Sep 12, 2023

Holding for Noetic sync

@github-actions
Copy link

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

@github-actions github-actions bot added the stale Issue/PR hasn't been active in a while and may be closed. label Sep 27, 2023
@sloretz sloretz removed the held for sync Issue/PR has been held because the distribution is in a sync hold label Oct 2, 2023
@github-actions github-actions bot removed the stale Issue/PR hasn't been active in a while and may be closed. label Oct 3, 2023
@sloretz sloretz added the held for sync Issue/PR has been held because the distribution is in a sync hold label Oct 10, 2023
@sloretz
Copy link
Contributor

sloretz commented Oct 10, 2023

Holding for Noetic sync

@github-actions
Copy link

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

@sloretz sloretz removed the held for sync Issue/PR has been held because the distribution is in a sync hold label Nov 16, 2023
@mjcarroll
Copy link
Member

@j-rivero I think @peci1 is still waiting on you here

@sloretz
Copy link
Contributor

sloretz commented Dec 7, 2023

Holding for Noetic sync

@sloretz sloretz added the held for sync Issue/PR has been held because the distribution is in a sync hold label Dec 7, 2023
@j-rivero
Copy link
Contributor

j-rivero commented Dec 7, 2023

@j-rivero I think @peci1 is still waiting on you here

Ouch sorry got distracted with some other things, I'll mail @peci1. The process of sponsoring, reveiew, new queue and wait for the FTP-masters can take some weeks, not sure if this affect to this issue or you want to leave it open until we complete the process.

@sloretz sloretz removed the held for sync Issue/PR has been held because the distribution is in a sync hold label Jan 11, 2024
@sloretz
Copy link
Contributor

sloretz commented Jan 23, 2024

Holding for Noetic sync

@sloretz sloretz added held for sync Issue/PR has been held because the distribution is in a sync hold and removed held for sync Issue/PR has been held because the distribution is in a sync hold labels Jan 23, 2024
@mjcarroll
Copy link
Member

@j-rivero and @peci1 any updates here?

@j-rivero
Copy link
Contributor

@j-rivero and @peci1 any updates here?

Code is ready and I've asked for sponsoring in Debian. Let you know here when we are ready.

@peci1
Copy link
Contributor Author

peci1 commented Feb 29, 2024

Thanks to @j-rivero the package has just been accepted to Debian unstable: https://packages.debian.org/sid/python3-http-relay .

It got already pulled to noble-proposed: https://launchpad.net/ubuntu/+source/http-relay/2.1.3-1 .

Now holding my breath whether it will make it into noble in time :) When that happens, I think we can move on with this PR?

@peci1 peci1 requested a review from a team as a code owner March 3, 2024 23:28
@github-actions github-actions bot added the rosdep Issue/PR is for a rosdep key label Mar 3, 2024
@peci1
Copy link
Contributor Author

peci1 commented Mar 4, 2024

So, the package has landed in Noble! https://packages.ubuntu.com/noble/python3-http-relay .

I've tried to update this PR according to the results of the previous discussion. However, I'm not sure I did it right and technically correct:

  • I've created a rosdep key pointing to APT python3-http-relay for noble+ and new debians, using pip for other systems, and having null for noetic and buster.
  • I've renamed the temporary ROS package to python3-http-relay and released it for Noetic (thus focal, maybe buster?).
    • The ROS package is now created by merging pip branch into master of https://github.com/ctu-vras/http_relay . master branch adds package.xml, CMakeLists.txt, removes pyproject.toml and changes setup.py.
  • The plan (in another PR) is to release it for all non-EOL'd ROS 2 distros and substitute the pip keys with the released version.

I'm not sure whether rosdep is clever enough to look for the key in rosdistro when it finds null in rosdep system package list. Tests are green, but I haven't actually tried it on Noetic. I can't figure out how to test rosdistro index changes locally without setting up whole buildfarm (I guess I'd need to regenerate the cache .gz files manually?)

@peci1
Copy link
Contributor Author

peci1 commented Mar 4, 2024

Or would it be better to do the release for older debian-based systems via reprepro?

@sloretz
Copy link
Contributor

sloretz commented Mar 19, 2024

Holding for Noetic sync

@sloretz sloretz added held for sync Issue/PR has been held because the distribution is in a sync hold and removed held for sync Issue/PR has been held because the distribution is in a sync hold labels Mar 19, 2024
@mjcarroll
Copy link
Member

CC: @nuclearsandwich or @cottsay. Do you have any input on @peci1's current state?

@cottsay
Copy link
Member

cottsay commented Apr 2, 2024

I did some local testing, and it appears that the rosdep database takes precedence over the ROS distribution index even when the rule is explicitly null, so this won't work as expected for Noetic.

$ rosdep resolve --rosdistro noetic --os ubuntu:focal python3-http-relay

ERROR: [python3-http-relay] defined as "not available" for OS version [focal]

[python3-http-relay] defined as "not available" for OS version [focal]
	rosdep key : python3-http-relay
	OS name    : ubuntu
	OS version : focal
	Data:
_is_ros: true
		arch:
		  pip:
		    packages:
		    - http-relay
		debian:
		  '*':
		  - python3-http-relay
		  bookworm:
		    pip:
		      packages:
		      - http-relay
		  bullseye:
		    pip:
		      packages:
		      - http-relay
		  buster: null
		fedora:
		  pip:
		    packages:
		    - http-relay
		gentoo:
		  pip:
		    packages:
		    - http-relay
		nixos:
		  pip:
		    packages:
		    - http-relay
		osx:
		  pip:
		    packages:
		    - http-relay
		rhel:
		  pip:
		    packages:
		    - http-relay
		ubuntu:
		  '*':
		  - python3-http-relay
		  focal: null
		  jammy:
		    pip:
		      packages:
		      - http-relay

EDIT:
Here's the spot in rosdep where the rules are merged. The comment states that the merge happens at the os_name level, so because there are ANY ubuntu rules in rosdep, the rule supplied by the distribution index are ignored.

@peci1
Copy link
Contributor Author

peci1 commented Apr 2, 2024

Uh-oh... thanks for the testing, @cottsay . So, would it be acceptable to change the behavior of rosdep? Or does anyone see a better approach?

@cottsay
Copy link
Member

cottsay commented Apr 2, 2024

I haven't looked into this deeply, but I don't see any downside to deepening the "merge" so that the ROS package can satisfy the nulled rule for focal specifically.

I don't think it will be trivial, though. We would need to handle os_code_name wildcards on either side of the merge, as well as explicit null on either side. Certainly new tests are appropriate.

@peci1
Copy link
Contributor Author

peci1 commented Apr 2, 2024

I'll have a look at it next week. Although a new behavior, I think it is desirable to add it because what I try to do with this package is the recommended way of adding packages that will get upstreamed to distros but need to be released in ROS repos for older distros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Maintainers have asked for changes to the pull request noetic Issue/PR is for the ROS 1 Noetic distribution persistent Issue/PR won't be marked as stale if inactive for a while. rosdep Issue/PR is for a rosdep key
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants