ros / geometry2

A set of ROS packages for keeping track of coordinate transforms.
190 stars 275 forks source link

Dependecy issues between tf2 and tf2_geometry_msgs #275

Open Crusty82 opened 6 years ago

Crusty82 commented 6 years ago

If you look closely, you can see that

tf2/include/tf2/impl/utils.h includes and tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.h includes

So there is a cycle there. I am forced to compile these packages via a different build system, and my system dies with a cyclic dependency error. Also note how tf2 sneakily includes tf2_geometry_msgs without actually requiring the package in CMakeLists or package.xml. I fail to understand how this can build in the first place, but it surely doesn't build for me.

tfoote commented 6 years ago

This was flagged in #170 There's a proposed fix to call it out specifically but that explicitly triggers the circular dependency in the main build. #170 so can't be merged. The main solution is that we need to move that implementation.

Crusty82 commented 6 years ago

Thanks for replying this quickly Tully, and sorry I missed issue #170. I will locally fix this then by moving functionality from tf2 into tf2_geometry_msgs. Thanks!

tfoote commented 6 years ago

No problem. If you have a clean solution a PR would be appreciated. I think it it's likely that the template specializations in the impl can move to tf2_geometry_msgs and then anyone with that as a dependency could use them the same way they always have.

trainman419 commented 5 years ago

I just ran into this too, and I also solved it by moving the functions that manipulate geometry_msgs types into tf2_geometry_msgs.

I can submit my changes as a PR if you'd like, but it's an API change and since I'm only testing against a small part of ROS and I'm not using cmake to build ROS, so I can't confirm that this fix is valid for the rest of the ROS ecosystem.

tfoote commented 5 years ago

@trainman419 If you have the moved implementation could you add a backwards compatible API placeholder that forwards to the new location and gives a deprecation warning? That deprecated API will still cause the dependency issue, but we can remove it in a future rosdistro and we can put a very specific warning that hopefully people will find the workaround quickly if they run into it.

mintar commented 5 years ago

:+1:

we can remove it in a future rosdistro

That rosdistro better be Noetic, since it's the last one! :)

jspricke commented 5 years ago

We got bugged by this in Debian as well: https://bugs.debian.org/916479. @trainman419 do you plan to send the PR soon?

trainman419 commented 5 years ago

Looks like my changes didn't end up being necessary for our project, and didn't make it into our repository. I will not be submitting a PR for these changes (I don't have any way to test the changes requested by @tfoote , either).

ayrton04 commented 3 years ago

This is causing one of our dev jobs to fail (though somehow the binary jobs are fine):

https://build.ros.org/job/Mdev__tf2_2d__ubuntu_bionic_amd64/1/consoleFull

17:55:25 /opt/ros/melodic/include/tf2/impl/utils.h:18:10: fatal error: tf2_geometry_msgs/tf2_geometry_msgs.h: No such file or directory
17:55:25  #include <tf2_geometry_msgs/tf2_geometry_msgs.h>
17:55:25           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Since tf2 doesn't (and can't) have tf2_geometry_msgs in its dependencies, tf2_geometry_msgs are not included in the build. I suppose the easiest thing for us to do is just include tf2_geometry_msgs as a dependency directly.

tfoote commented 3 years ago

Yes, if you include tf2_geometry_msgs as a dependency it will be fine. You should do this if you use utils.h which is the only place that uses this. It's header only and should probably be refactored out.

jspricke commented 3 years ago

@tfoote I did the refactoring in #357 would be great to get some feedback on this.