search

ros-drivers / freenect_stack

libfreenect based ROS driver
http://ros.org/wiki/freenect_stack
35 stars 52 forks source link

Missing log4cxx headers when building against latest ros_comm. #10

Closed piyushk closed 10 years ago

piyushk commented 10 years ago

The deb build is currently broken. See https://github.com/ros-drivers/hokuyo_node/issues/7 for context.

Solution pending https://github.com/ros-drivers/hokuyo_node/pull/8

piyushk commented 10 years ago

http://jenkins.ros.org/job/ros-hydro-freenect-camera_binarydeb_precise_amd64/29/console

piyushk commented 10 years ago

See PR2 fix here: https://github.com/PR2/pr2_ethercat_drivers/commit/95fc95d4c117042afb7ebb5db72f5ca741e0bc1c

Add a version_gte attribute for rosconsole in the manifest, and set it to the current version of rosconsole.

trainman419 commented 10 years ago

I think the proper fix for this is here: https://github.com/ros/ros_comm/pull/336

dirk-thomas commented 10 years ago

The ros_comm pull request has been merged and released.

Please address the missing explicit dependency on log4cxx to unbreak the build on the farm.

jack-oquin commented 10 years ago

Removing a dependency from a fundamental interface should never have been done after the Hydro release freeze.

Let's chalk this one up to experience, and resolve never to do anything like it again.

dirk-thomas commented 10 years ago

@jack-oquin I disagree with you on this. A package should never rely on transitive dependencies. Whenever source code uses something external it must declare that dependency explicitly itself.

piyushk commented 10 years ago

@dirk-thomas I agree with your point that this was a bug, but I agree with Jack that only necessary bug-fixes should be done post-release, unless the package is in active development. In my opinion, removing the transitive dependency was not a necessary bug-fix.

jack-oquin commented 10 years ago

I don't have a problem with fixing a missing dependency.

I do object strongly to making API-breaking changes in released code. That mistake turned a bunch of hidden bugs into actual bugs.

jack-oquin commented 10 years ago

Why not just fix rosconsole in Hydro and remove the no-longer-needed dependency in Indigo?

That is how distributions are supposed to work.

dirk-thomas commented 10 years ago

I do understand that the changes in this case imply effort for other package maintainers. Usually we try to keep them as small as possible. We did merge it in the assumption that it would not require any changes downstream. That assumption did not hold but the reason was that the downstream code was (I would consider it) buggy. If we would have known before how many packages did not state their dependencies correctly we might have decided otherwise.

@jack-oquin We did release the change into Hydro since it did not change API. A transitive dependency is simply not something you can rely on - you should consider that non public API. Dependencies are only provided downstream when required for headers or libraries. The code blocks in question are using log4cxx symbols directly - not through any ROS API.

jack-oquin commented 10 years ago

@jack-oquin We did release the change into Hydro since it did not change API. A transitive dependency is simply not something you can rely on - you should consider that non public API. Dependencies are only provided downstream when required for headers or libraries. The code blocks in question are using log4cxx symbols directly - not through any ROS API.

@dirk-thomas: If we were having this discussion in the context of an upcoming release like Indigo, I would fully agree with your statement. But, talking about Hydro is a completely different matter.

I would guess that 90% of all released ROS packages have some dependency errors. As a practical matter, removing a dependency in a released package is a breaking API change. Release stability is more important than fixing theoretical errors, or arguing about what is part of the API and what is not.

So, let's agree not to do that any more.

jack-oquin commented 10 years ago

My first attempt to fix this (above) failed.

I did not realize that the log4cxx symbols that had apparently been defined for years in the ros/ros.h include hierarchy had suddenly been removed. Since that change is not present in the currently-released ros_comm, it was hard to detect before releasing.

jack-oquin commented 10 years ago

freenect_camera-0.3.2 seems to build now.

dirk-thomas commented 10 years ago

Removing the dependency was not a "theoretical error" we fixed just for the heck of cleaning up stuff. The logging has been refactored to allow to use different logging backends: log4cxx, glog, console-only or any custom implementation of that interface.

jack-oquin commented 10 years ago

All worthy enhancements, I am sure.

They should have waited for Indigo.