ros-drivers / openni2_camera

ROS wrapper for openni 2.0
http://wiki.ros.org/openni2_camera
BSD 3-Clause "New" or "Revised" License
56 stars 96 forks source link

Move openni2_launch package into this repository #55

Closed 130s closed 6 years ago

130s commented 6 years ago

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 https://github.com/ros-drivers/openni_launch/issues/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 commented 6 years ago

see https://github.com/ros-drivers/openni_launch/issues/30#issuecomment-335277899 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.

:+1: for keeping the history of migrated files

130s commented 6 years ago

Same as https://github.com/ros-drivers/openni_launch/issues/30#issuecomment-338225649, 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 commented 6 years ago

@ros-pull-request-builder retest this please

130s commented 6 years ago

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 commented 6 years ago

As explained in the question https://github.com/ros-drivers/openni2_camera/pull/55#issuecomment-340272365, failed status on Github for ROS buildfarm CI doesn't seem to be critical in this particular case. Merging.