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

Package update to work with hydro with no user intervention #6

Open
wants to merge 1 commit into
base: hydro-devel
Choose a base branch
from

Conversation

progtologist
Copy link

Updated CMakeLists to download OpenNI2 and NiTE2 and copy headers and libraries to the required folders locally (no root permissions).

Updated the manifest.xml .

Made minor changes on the source code to publish the transformation of the identified users.

Tested on Ubuntu 12.04 64bit, should also work on 32bit.
Additional changes needed for Windows or OSX users.

…downloads the libraries and installs them locally, publishes transformations for the identified users
ExternalProject_Add(
OpenNI2
PREFIX ${CMAKE_CURRENT_BINARY_DIR}
URL http://www.openni.ru/wp-content/uploads/2013/01/OpenNI-Linux-x64-2.1.0.tar.zip
Copy link
Member

Choose a reason for hiding this comment

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

This is not acceptable in my opinion, it should be downloaded from the structure.io website or better yet OpenNI's GitHub:

@wjwwood
Copy link
Member

wjwwood commented Jan 13, 2015

Thank you for the contribution, but I must give this approach a -1, for a few reasons:

  • It distributes precompiled binaries which will not work on ARM, for example.
  • It downloads source and precompiled binaries from a website other than GitHub.
  • It downloads a fixed version of the software every time rather than embedding it or releasing it separately.
    • In both cases you'll always get the same version of OpenNI, but if you embed it then you don't have to download it every time, i.e. you're not downloading from a branch which can change over time.
    • You're not (or shouldn't be) downloading large binary files, just the source, so embedding it in the repository is practical.
    • This is always required to build, i.e. downloading is not optional, as far as I can see.

I would go so far as to say, that if the maintainers intend to merge this and release it, I will not accept it to the ROS build farm due to the downloading of source from somewhere other than the projects official source repository (on GitHub).

@progtologist
Copy link
Author

The problem with openni2_tracker package is that it depends on OpenNI2 and NiTE2, which where removed from the official site when PrimeSense was acquired by Apple.

This caused many issues and in result caused developers to stop actively contributing. This package, no matter how one can wrap it, cannot be officially released, as NiTE2 is not in the debian repositories.

So why did I propose this PR? Well, since you cannot install the package from the build farm, at least it would be beneficial for the user base to be able to simply 'catkin_make' it without any additional installation steps.

To answer your reasons one by one:

  • I could provide additional checks to distribute precompiled binaries for ARM. I have however no fast way of testing it.
  • The OpenNI github page only provides OpenNI version 1, for this package both OpenNI2 and NiTE2 are required. Even if we provided an alternative OpenNI2 link (structure.io), NiTE2 is still only provided by openni.ru link to answers.ros.org
  • I cannot embed or release OpenNI2 or NiTE2, since I do not have the licence to do so.
    • ExternalProject only downloads the file if it does not already exist, otherwise those steps are omitted by cmake.
    • Source is not available for NiTE2.
    • Downloading is only done once or if you delete the build folder.

Overall, I agree that this package should not be released. However this does not change the current status of the package, it is because of the NiTE2 dependency that we cannot release it. We should however provide users an easy way to install openni2_tracker.

@wjwwood
Copy link
Member

wjwwood commented Jan 16, 2015

I apologize, I didn't have all the details straight before, but I still would not recommend this approach. I understand the motivation for the pull request, but even after your response I still see a few problems. However, I believe that this can be worked into shape such that it can be released, as long as we figure out something for the NiTE problem.

I could provide additional checks to distribute precompiled binaries for ARM. I have however no fast way of testing it.

I would argue that you should always build the target from source, unless the source is not available, that way you don't have a problem on ARM and other platforms (e.g. OS X). Is it the case that the OpenNI2 source is not available? I see that you pointed out that the NiTE2 source is not available has that always been the case or is it a recent development?

The OpenNI github page only provides OpenNI version 1, for this package both OpenNI2 and NiTE2 are required. Even if we provided an alternative OpenNI2 link (structure.io), NiTE2 is still only provided by openni.ru link to answers.ros.org

The OpenNI2 code is on GitHub: https://github.com/occipital/openni2

I cannot embed or release OpenNI2 or NiTE2, since I do not have the licence to do so.

AFAICT, the license for OpenNI2 is still Apache 2.0, which would allow this. For NiTE, the license page is no longer hosted so I don't know what it says. I would recommend having people install NiTE themselves, or at the very least include the license for NiTE in this package, including in the package.xml's license field. Because whether they know it or not people will be linking against NiTE and they may not even know that it is part of this process. That's why it would be so much better to have NiTE distributed separately in it's own deb or at the very least in it's own ROS package.

Downloading is only done once or if you delete the build folder.

This is a fairly common practice, but if the NiTE source is not available then you have little choice. OpenNI2 is a different story.

I'm pinging you @tfoote since you are the maintainer of the old nite ROS package: http://wiki.ros.org/nite

@tfoote
Copy link
Contributor

tfoote commented Jan 16, 2015

NITE is binary only. But OpenNI has been compiled from source all along.

@bit-pirate
Copy link
Member

For the "NITE-problem" I like to suggest the same approach recently implemented in the old openni_tracker: ros-drivers/openni_tracker#8

Regarding OpenNI, I guess someone either has to release it as a Debian package or we apply the same approach as for NITE (warning if not installed + (link to) install instructions).

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.

4 participants