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

Merge libphidgets branch into indigo #18

Merged
merged 6 commits into from
Mar 23, 2015
Merged

Merge libphidgets branch into indigo #18

merged 6 commits into from
Mar 23, 2015

Conversation

muhrix
Copy link
Collaborator

@muhrix muhrix commented Mar 23, 2015

The changes within libphidgets branch allowed the phidgets_drivers package to avoid downloading and installing a third party library, depending on the existing libphidgets package (which is released as a .deb package).

muhrix added a commit that referenced this pull request Mar 23, 2015
Merge libphidgets branch into indigo
@muhrix muhrix merged commit 8d25dfb into indigo Mar 23, 2015
@v4hn
Copy link

v4hn commented Mar 27, 2015

Ok,

I agree it's a good idea to move libphidget out of this module. It's a (non-ROS) module on it's own.
However, ros-indigo-libphidgets is cob's attempt to package libphidget within ROS and it has multiple flaws.

  • the version is quite outdated and even older than the one used in this project before (which is outdated as well)
  • they package the library as a ROS module, when it is not (and indeed, the way you find_package it now via "find_package(catkin COMPONENTS libphidgets)" is obsolete for years)
  • they replace the library's package-config file by one with a different name and less content. This makes it impossible to use a system installed version of libphidget when you depend on their module.

The sane detection of libphidget should imho look like this: v4hn@98d6e8d
However, this doesn't work with ros-indigo-libphidgets for the reasons given above.

So what's the way to go here? Create a new (proper) libphidget21 module?
Add a second way to detect libphidget they way I implemented it in my fork?
Try to improve cob's module?

@mintar
Copy link
Contributor

mintar commented Mar 27, 2015

@v4hn : I fully agree that libphidget should be a system library, not a ROS package. However, if we want to release phidgets_drivers using the ROS build farm, we need libphigets as either:

  1. a ROS package,
  2. a package that's in the official Ubuntu repos for the targeted Ubuntu version, or
  3. a (non-ROS) deb package that's also distributed via packages.ros.org.

The cleanest way would be (3): manually do the deb packaging for libphidgets ourselves (like @JochenSp did for libepos), then cajole the OSRF people into uploading it onto packages.ros.org.

Sounds like a lot of work though (especially the cajoling part).

Do you need any new features/bug fixes from more recent versions of libphidgets? If so, it would probably be easiest to get the cob people to update their libphidgets ROS package.

@v4hn
Copy link

v4hn commented Mar 27, 2015

As it's unlikely that libphidget (no "s", that was added by cob) will
become an official package in ubuntu 14.04, I agree that 3. is the ideal way to go.
And yes, it is a bit of work, that's why I was pretty happy nobody touched
the existing construction...

On Fri, Mar 27, 2015 at 07:20:33AM -0700, Martin Günther wrote:

Do you need any new features/bug fixes from more recent versions of libphidgets?

I mostly wanted to point out that you now depend on an obsolete and
incompatible module to provide libphidget instead of building it yourself
(and this module builds libphidget quite the same way you did in here,
only worse afaics). I don't think this is an improvement.

As this project tries to provide drivers for phidgets hardware,
I believe it should also include a (If you think it's more easy)
ROS-libphidget module on its own. As COB only provides that package
as "cob_extern", I think they would not protest if you take care
of supporting a libphidget ros wrapper.

@v4hn
Copy link

v4hn commented Mar 27, 2015

referencing @ipa-fmw as someone from who seems to be responsible for the cob_extern/libphidgets module

@muhrix
Copy link
Collaborator Author

muhrix commented Mar 30, 2015

Very interesting points being discussed here! :-)

@v4hn, the changes were made to avoid conflicts with an existing release of libphidget and be compliant with REP 136, and hence the external library had to be removed. We've got a working release which does not conflict with cob_extern, with the sole disadvantage of a slightly older libphidget version.

Given that cob_extern released libphidgets as a ROS module (not a system library), using "find_package(catkin COMPONENTS libphidgets)" is the correct way of adding build flags from another catking package (see documentation). This is neither obsolete nor incompatible (unless you're talking about how one should release third party libraries).

I think @mintar summarised very well the options we have.
Option 3 would be ideal, thus it would be great if someone could contribute towards the development of this solution!
Also, the outdated version used in cob_extern is only a pull request away for the package to use the latest libphidget version.

I do agree that this repo/package is where libphidget should be, rather than cob_extern. However, as I previously mentioned, cob_extern was already released. If COB are happy dropping their libphidgets so that we take over release and maintenance of the library, then we can do it from here (though I would still go for option 1 for reasons given by @mintar). Otherwise, I would rather keep the release as it is.

I'll open an issue over at cob_extern to discuss the matter.

Suggestions and contributions are more than welcome!

On a side note, perhaps what the guys who released cob_extern thought was "maybe someone installed the official libphidget system-wide from source, and we do not want to mess up with that", and hence they released it as a ROS module, which is "sandboxed" within ROS -- just a thought.

@v4hn
Copy link

v4hn commented Mar 30, 2015

On Mon, Mar 30, 2015 at 03:40:36AM -0700, Murilo FM wrote:

be compliant with REP 136

Sure, that would be good. But as the cob package fails to comply,
I don't think you are compliant yet. :)

Given that cob_extern released libphidgets as a ROS module (not a system library), using "find_package(catkin COMPONENTS libphidgets)" is the correct way of adding build flags from another catking package

You're right in that, given that cob uses catkin_package inside their CMakeLists.txt and don't install libphidget's original pc file (both violates the REP you cited), it's "correct"(actually the only way) to include their libphidget(s).

Also, the outdated version used in cob_extern is only a pull request away for the package to use the latest libphidget version.

That still doesn't allow me to use a system installation of libphidget
instead of their package and imho this is the bigger problem.

If COB are happy dropping their libphidgets so that we take over release and maintenance of the library,
then we can do it from here
[...]
I'll open an issue over at cob_extern to discuss the matter.

Thanks for opening that issue!

(though I would still go for option 1 for reasons given by @mintar).

As I don't run a debian-style distribution over here, I can't really help with a deb-package.
However, libphidget is a small library and is easy to package. Would be great to see that solution implemented.

@muhrix
Copy link
Collaborator Author

muhrix commented Mar 30, 2015

Given that you are having to build the package from source anyway, I believe the best approach would be what you did in your fork (at least until we sort this out with COB).

In theory, phidgets_drivers has nothing to do with REP 136 as of this current release, since it does not release a third party library.

I'm hoping the guys who maintain cob_extern will be happy to move the library over to phidgets_drivers, so that we can figure out a better way to distribute the library.

@v4hn
Copy link

v4hn commented Mar 30, 2015

On Mon, Mar 30, 2015 at 06:37:40AM -0700, Murilo FM wrote:

Given that you are having to build the package from source anyway, I believe the best approach would be what you did in your fork (at least until we sort this out with COB).

The problem is that my fork doesn't work with Ubuntu and COB's libphidgets at the moment,
because they fail to install libphidget21.pc, which is the official way to detect libphidget.

I'm hoping the guys who maintain cob_extern will be happy to move the library over to phidgets_drivers,
so that we can figure out a better way to distribute the library.

Yes, that would be great.

@muhrix
Copy link
Collaborator Author

muhrix commented Mar 30, 2015

Some updates on this issue...

I contacted Phidgets regarding the packaging and distribution of their library and I have been told that there are some people currently working on the packaging for Debian.

I'll get in touch with them.

@gavanderhoorn
Copy link

A small comment on the remark that "the official way would be to use PkgConfig to find libphidget".

Afaik, the CMake preferred way to deal with libraries that don't provide a find script is to contribute one yourself or write a CMake Config file (or the new page) and get either of those accepted upstream. If that is not an option, then you can use PkgConfig, but should be aware of the potential issues.

An alternative might be to exploit Catkin's support for adding additional patches to the release repository to get your config file distributed with your 3rd-party pkg.

I do agree though that the current pkging in cob_extern is far from ideal and should be fixed.

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.

None yet

4 participants