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

Move openni2_launch package into this repository #55

Merged
merged 38 commits into from
Nov 3, 2017

Conversation

130s
Copy link
Member

@130s 130s commented Oct 9, 2017

openni2_camera package only contains node and nodelet library, and I assume most of its users use it via openni2.launch from openni2_launch package, which is maintained in another repo separately. IMO these two repos were separately started during the very early development phase, which made sense at that time. Now that development has slowed down, we can merge two repos into one to reduce maintenance effort.

Very similar suggestion is made at ros-drivers/openni_launch#30.

Implementation
I'd suggest simply moving openni2_launch pkg into openni2_camera repo, which is already done in this PR. Folder structure:

openni2_camera$ tree -L 2
.
├── openni2_camera
│   ├── cfg
│   ├── CHANGELOG.rst
│   ├── CMakeLists.txt
│   ├── include
│   ├── openni2_nodelets.xml
│   ├── package.xml
│   ├── README.md
│   ├── ros
│   ├── src
│   ├── srv
│   └── test
└── openni2_launch
    ├── CHANGELOG.rst
    ├── CMakeLists.txt
    ├── launch
    └── package.xml

9 directories, 8 files

Commit history for the new package should be all preserved from its repo except the merge commits. To retain all commit history, the following set of commands is used (reference):

$ cd openni2_camera
$ git checkout -b merge/openni2_camera
$ mkdir openni2_camera
$ mv * openni2_camera/
$ git add openni2_camera
$ git status
$ git rm C* launch/* p*
$ git commit -m "openni2_camera package as a subfolder."

$ cd ../openni2_launch
$ mkdir openni2_launch
$ mv * openni2_launch/
$ git add openni2_launch/
$ git rm C* launch/* p*
$ git commit -m "openni2_launch package as a subfolder."
$ git log --pretty=email --patch-with-stat --reverse --full-index --binary -- . > /tmp/patch

$ cd ../openni2_camera
$ git am < /tmp/patch

Future plan for the external repo
Once this PR gets merged, https://github.com/ros-drivers/openni2_launch should be deprecated. Existing issues can be automatically imported into this repo.

@mikaelarguedas
Copy link
Contributor

see ros-drivers/openni_launch#30 (comment) for a suggestion of keeping indigo-devel the same and migrate only for newer distro to make sure not to break LTS users, people building from source on EOL distros or people relying in the current location of the code.

👍 for keeping the history of migrated files

@130s
Copy link
Member Author

130s commented Oct 24, 2017

Same as ros-drivers/openni_launch#30 (comment), which I am citing below for the record, I suggest to move on with this PR as is.

If no concern will be received in a week, I'll merge this.

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 130s force-pushed the merge/openni2_launch branch 2 times, most recently from b28a68f to 6ebf8f6 Compare October 29, 2017 15:16
@130s
Copy link
Member Author

130s commented Oct 29, 2017

@ros-pull-request-builder retest this please

@130s
Copy link
Member Author

130s commented Oct 29, 2017

CI passed except for the one on ROS buildfarm. Its result only says unstable if you look at its result. I asked a question for clarification.

@130s
Copy link
Member Author

130s commented Nov 3, 2017

As explained in the question #55 (comment), failed status on Github for ROS buildfarm CI doesn't seem to be critical in this particular case. Merging.

@130s 130s merged commit c8274f8 into ros-drivers:indigo-devel Nov 3, 2017
@130s 130s deleted the merge/openni2_launch branch November 3, 2017 12:45
130s added a commit to 130s/openni2_launch that referenced this pull request Nov 3, 2017
130s added a commit to 130s/openni2_launch that referenced this pull request Nov 3, 2017
130s added a commit to 130s/rosdistro that referenced this pull request Nov 4, 2017
Repository merged into another repo (detail ros-drivers/openni2_camera#55).

New releases for IKL from the new repo will become ready to be made, as soon as this PR gets merged. But still we cannot avoid "downtime" of `openni2_launch` in rosdistro.

I'll try to stay on alert but if I can't repond, someone with write access to the release repo can make a new release. Artifacts are already pushed so just run:

  bloom-release --rosdistro indigo --track indigo openni2_camera -p
  bloom-release --rosdistro kinetic --track kinetic openni2_camera -p
  bloom-release --rosdistro lunar --track lunar openni2_camera -p
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.