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

tf2 in groovy defines unnecessary CATKIN_DEPENDS (e.g. on rospy) #18

Closed
tkruse opened this issue Jul 29, 2013 · 3 comments
Closed

tf2 in groovy defines unnecessary CATKIN_DEPENDS (e.g. on rospy) #18

tkruse opened this issue Jul 29, 2013 · 3 comments

Comments

@tkruse
Copy link
Member

tkruse commented Jul 29, 2013

I checked the .pc files generated for a fresh groovy source install.

I get:

$ find devel_isolated/ -name *.pc | xargs grep Req | grep rospy
devel_isolated/diagnostic_aggregator/lib/pkgconfig/diagnostic_aggregator.pc:Requires: diagnostic_msgs pluginlib roscpp rospy xmlrpcpp
devel_isolated/tf2_ros/lib/pkgconfig/tf2_ros.pc:Requires: actionlib actionlib_msgs geometry_msgs message_filters roscpp rosgraph rospy tf2 tf2_msgs
devel_isolated/tf2/lib/pkgconfig/tf2.pc:Requires: geometry_msgs tf2_msgs rospy roscpp
devel_isolated/interactive_markers/lib/pkgconfig/interactive_markers.pc:Requires: roscpp rosconsole rospy tf visualization_msgs

I am not 110% sure, but I believe there is no need ever for a library to know of rospy to build against tf2, diagnostic_aggregator or interactive_markers. So it seems to me those have flawed CMakeLists.txt

Possibly more exported dependencies than rospy are superfluous there (e.g. actionlib stuff).

Also see:
http://answers.ros.org/question/58498/what-is-the-purpose-of-catkin_depends/

I am too lazy to create individual tickets for each of the packages above, especially since I might be wrong.

@tfoote tfoote closed this as completed in bed29af Aug 3, 2013
@tfoote
Copy link
Member

tfoote commented Aug 3, 2013

fixed in groovy 6c8ea39

@tkruse
Copy link
Member Author

tkruse commented Aug 3, 2013

I also miss boost as DEPENDS, used in https://github.com/ros/geometry_experimental/blob/hydro-devel/tf2/include/tf2/buffer_core.h
On the other hand, I am not sure whether console_bridge should be exported as dependency, given it's not used in the public headers.

I could go on.

E.g.
tf2_bullet: misses geometry_msgs and tf2 as exports
tf2_geometry_msgs: tf2_ros is exported but not used
tf2_kdl: misses geometry_msgs (both as find_package as well as in catkin_depends), and tf2 as catkin_depends

Correct me if I am wrong, but a review process for the catkin_package macro should go like this:

  1. Check the package author did not do something unorthodox and installs headers from somewhere else than /include//*.h (if he did, this should probably in itself be a style ticket, in conventional packages)
  2. Look what is used in the /include//*.h files (and only those)
  3. Make sure all packages mentioned there are declared in catkin_package as DEPENDS or CATKIN_DEPENDS
  4. Anything in catkin_package not mentioned in any of the headers should go, unless it's something exceptional (genmsg, not sure what else would count)

The more packages get this worng, the more people will be confused and annoyed about catkin. I think except for packages catkinized by maybe Dirk or Vincent, most packages get the DEPENDS and CATKIN_DEPENDS wrong. Maybe that is a hint that those keywords are not helpful and self-explaining, but confusing?

@tkruse
Copy link
Member Author

tkruse commented Aug 4, 2013

Note I am not just talking about the tf2 packages here, I am talking about hydro in general. Sampling over at a few catkin packages in hydro, I see many packages flawed that way, and that is a major problem in the general introduction of catkin.

daju1 pushed a commit to daju1-ros/geometry-experimental that referenced this issue Oct 28, 2016
dhood pushed a commit to dhood/geometry2 that referenced this issue May 30, 2017
Since geometry_msgs::msg::TransformStamped is declared with
an empty constructor, all of its fields are uninitialized.
This means that anytime we want to use it, we must initialize
all fields ourselves.  The tests in simple_tf2_core.cpp were
failing to do this, so parts of tf2 were making decisions on
basically random data.  This was causing a bunch of valgrind
failures.  With this commit, the test becomes valgrind clean.

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
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

No branches or pull requests

2 participants