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

Consolidate rgbd repositories into a single repo #30

Closed
130s opened this issue Apr 24, 2017 · 17 comments
Closed

Consolidate rgbd repositories into a single repo #30

130s opened this issue Apr 24, 2017 · 17 comments

Comments

@130s
Copy link
Member

130s commented Apr 24, 2017

The following repositories contain a single package. I suggest to create a new repo where we move all of these 3 packages to, in order to reduce the maintenance effort.

  1. openni_launch
  2. rgbd_launch
  3. openni_camera

Background
Despite the inactive development, the release tasks for these packages still takes certain amount of work. For Lunar this time, I took charge of it and for total it took almost an hour, mainly because these packages are inter-depended and I had to make release one by one and wait for the binary to become ready, in order for bloom to be able to resolve the necessary dependency.

Suggested solution
Create a new repo and move all of these 3 packages there.
Suggestion for the repo name appreciated. Here I temporarily use openni_meta.

New repo and the contents (being updated over time taking suggestions in):

ros-drivers/openni_meta
|- freenect_camera
|- freenect_launch
|- openni_camera
|- openni_launch
|- openni_meta
|- rgbd_launch

openni_meta is a meta package that run_depends on these 3 packages.

@piyushk
Copy link
Contributor

piyushk commented Apr 24, 2017

I understand the issues with extra maintenance effort. If you would prefer, you can also merge freenect_* into a meta repo as well.

I also recommend calling the repository rgbd_meta.

@130s
Copy link
Member Author

130s commented Apr 24, 2017

@piyushk I'm only doing the release tasks for these packages so feedback from actual developers like you is appreciated.

Do you mean to include freenect_* into the same single repo where openni_* and rgbd_* are, and that repo name can be rgbd_meta?

@piyushk
Copy link
Contributor

piyushk commented Apr 24, 2017

@130s Thanks for spending time releasing!

Yes, that is exactly what I meant!

@130s
Copy link
Member Author

130s commented Apr 25, 2017

I see.

@piyushk could you provide one more feedback? A caveat I can think of by the unified release from the aggregated repo is that all packages need to align the same package version. The current ersions taken from the main branch of each repo:

I propose either:

  • 2.2.0 (adjusting to the highest available, rgbd_launch, and incremented minor ver)
  • 3.0.0 (similar to adjusting to the highest available but incremented major ver for simplicity)

@piyushk
Copy link
Contributor

piyushk commented Apr 25, 2017

That's the only repository. There are multiple packages inside it.

I would recommend 2.1.4 across all of them, since this change maintains backward compatibility. However, given that other numbers are going to change, 2.2.0 sounds good to me.

@130s 130s changed the title Aggregate repositories openni_{camera, launch}, rgbd_launch into a single repo Consolidate rgbd repositories into a single repo Apr 26, 2017
@mintar
Copy link
Contributor

mintar commented May 30, 2017

What about openni2_camera and openni2_launch? openni2_launch also depends on rgbd_launch, so by that logic it should also go into that merged repo.

Personally, I'm not convinced of the proposal to merge everything together. Especially openni2_camera, openni_camera and freenect_stack don't seem to have any inter-dependencies (please correct me if I'm wrong). It might be more convenient to release everything in one go, but I think this is outweighed by the loss of clarity when trying to pinpoint a bug in one mega-repository.

Also, there are third-party packages for cameras such as the Orbbec Astra that include rgbd_launch. It's a bit weird for them to require the freenect, openni and openni2 libraries just to include some launch file.

Just my 2 cents, it's up to you maintainers to decide. :)

@skohlbr
Copy link

skohlbr commented Aug 21, 2017

Note also the Intel Realsense integration (https://github.com/intel-ros/realsense) uses rgbd_launch, so forcing installation of all openni-related s/w appears undesirable.

@130s
Copy link
Member Author

130s commented Oct 9, 2017

Thanks for the inputs!

It's a bit weird for them to require the freenect, openni and openni2 libraries just to include some launch file.

If the developers of downstream driver packages (say Astra's ROS pkg) want to build the upstream driver packages from source, yes they would clone unnecessary codes in addition, which I realized a bit less useful/user-friendly (or whatever appropriate way you call) indeed.
(Meanwhile, if they can use binary, then obviously they could only use the specific binary pkg they need.)

How about coupling only those that makes more sense?
E.g. I found freenect_stack is a good example where driver and launch packages sit together.

freenect_stack
|- freenect_camera
|- freenect_launch
|- freenect_stack

So, new proposed layout (top level is a repository):

openni_camera
|- openni_camera
|- openni_launch

openni2_camera
|- openni2_camera
|- openni2_launch

rgbd_launch

For openni2, I already opened separately a PR ros-drivers/openni2_camera#55 to merge into the existing openni2_camera repo.

Not sure how necessary meta packages are for these consolidation, since the dependency is only one-way from launch to driver, not the other way round, so that even those who build from source rosdep should be helpful to systematically install dependency.

@mintar
Copy link
Contributor

mintar commented Oct 9, 2017

So, if I understand you correctly, you're now proposing to merge openni_camera and openni_launch into one repo, openni2_camera and openni2_launch into a second repo, and keep rgbd_launch separate? That makes sense to me.

@130s
Copy link
Member Author

130s commented Oct 9, 2017

@mintar Yes. Only change I now suggest after all is consolidating openni_* packages.

@piyushk @skohlbr I appreciate your input.

@piyushk
Copy link
Contributor

piyushk commented Oct 9, 2017

@130s: Your proposed solution sounds good to me, and I think conforms with the other feedback provided. 👍

@mikaelarguedas
Copy link

Thanks @130s for the initiative!
From recent experience in shuffling code across repositories. It's hard to estimate how many people rely on the current location of the code by cloning hard-coded url in various scripts. The easiest way to move forward without breaking earlier distros is to branch out (kinetic-devel or lunar-devel) and keeping the current upstream branch in the same state (this way EOL distros like Jade can still be built from source). The new branch can be cleaned of the code and add a deprecation + redirection in the README.
It may also be valuable to announce it on discourse so that people relying on the current layout and not watching this repository can be notified before the migration.

@130s
Copy link
Member Author

130s commented Oct 13, 2017

@mikaelarguedas
Thanks for sharing the insight. Still trying to understand, if we strictly apply what you're suggesting, should we do this in the next distro (M) since there might be already users who're pointing to the current latest branch?

@mikaelarguedas
Copy link

@130s If we want to go all the way yeah we would wait until ROS-M. Then its a judgement call. I'm usually trying to not make any potentially breaking change in LTS distros but allow myself to try out changes that I think are beneficial on non-LTS without waiting for the next distro to be out. So in this case I'd branch out for lunar and consolidate the repos on lunar-devel but keep indigo and kinetic as is to make sure not to break anything. Then as I said it's up to the maintainer

@130s
Copy link
Member Author

130s commented Oct 20, 2017

I see. That by itself makes sense.

Then with making multiple branches for live distros, obviously we'd need to maintain branches (cherry-picking etc.) Making new releases from multiple branches also adds up more work no matter how small it is. In addition personall, primary motivation for my maintenance contribution is for Indigo as my main project is still on Indigo. With all of these, I'd suggest:

  • Freeze an old repo (openni_launch). Add deprecation notice and navigate to a new consolidated repo.
  • Make a new release from the new repo only.
  • New commits are only merged into the new repo.

This way, existing usecases shouldn't be broken. Only (minor) caveats:

  • a. those who use openni_launch by source won't receive any new changes unless they switch to point to the new repo
  • b. there will be 2 locations where you find openni_launch/indigo-devel branch.
  • c. Registered location in rosdistro will be updated.

I think these can be mitigated by making proper announcement and adding a notification at the old repo.

130s added a commit to 130s/openni_camera that referenced this issue Jan 4, 2018
…drivers/openni_launch.

See ros-drivers/openni_launch#30 (comment) for the decision for this change.

Preserving commit history was attempted (using the method in http://www.pixelite.co.nz/article/extracting-file-folder-from-git-repository-with-full-git-history), but didn't seem to easily work due to ros-drivers/openni_launch#4 that resurrects the change made previously in a few files and somehow `git am` command stops. So unfortunately, instead of spending more time on trying to save the commit history, I suggest to move forward.
@130s
Copy link
Member Author

130s commented Jan 6, 2018

Thank you for the discussion. Closing as I've done pretty much all moving chore:

@130s 130s closed this as completed Jan 6, 2018
@130s
Copy link
Member Author

130s commented Jan 6, 2018

I disabled the "issue" feature on this repo to avoid new issues, but apparently it will disabled access to all existing issues regardless open or not. So I re-enabled the issue.

Also, I could import closed issues if I wanted to, but that would send many spams to the subscribers on the new repo so I refrained.

tfoote pushed a commit to ros/rosdistro that referenced this issue Jan 10, 2018
* [IKL] Merge openni_{camera, launch} into a single repo.
Following the decision in ros-drivers/openni_launch#30

* [IKL] openni_camera version adjust to release repo.

* [openni_camera][lunar] Disable source test.
As ros-drivers/openni_camera#56 (comment), test might always fail on GitHub for UNSTABLE, which makes the maintenance harder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants