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.

Sign up for GitHub

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

Jump to bottom

Dependecy issues between tf2 and tf2_geometry_msgs #275

Open
Crusty82 opened this issue Dec 7, 2017 · 11 comments · May be fixed by #357
Open

Dependecy issues between tf2 and tf2_geometry_msgs #275

Crusty82 opened this issue Dec 7, 2017 · 11 comments · May be fixed by #357
Labels
bug

Comments

@Crusty82
Copy link

Crusty82 commented Dec 7, 2017

If you look closely, you can see that

tf2/include/tf2/impl/utils.h includes <tf2_geometry_msgs> 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
Copy link
Member

tfoote commented Dec 7, 2017

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
Copy link
Author

Crusty82 commented Dec 7, 2017

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
Copy link
Member

tfoote commented Dec 7, 2017

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.

@tfoote tfoote added the bug label Mar 22, 2018
@tfoote tfoote mentioned this issue Mar 22, 2018
Closed
@mikeferguson mikeferguson mentioned this issue Jul 30, 2018
Merged
mikeferguson added a commit to ros-planning/navigation that referenced this issue Jul 30, 2018
@mikeferguson
fix rotate_recovery debian build
4ff9bd7 
* add depend on tf2_geometry_msgs (due to ros/geometry2#275)
* add other hidden depends: angles, geometry_msgs, tf2
mikeferguson added a commit to ros-planning/navigation that referenced this issue Jul 31, 2018
@mikeferguson
fix depends for dwa_local_planner
aa2a0ea 
* add tf2_geometry_msgs (due to ros/geometry2#275)
* add missing depends on angles, sensor_msgs, tf2
mintar added a commit to ros-planning/navigation_experimental that referenced this issue Sep 5, 2018
@mintar
twist_recovery: Add missing dependency
193861d 
Actually, this is a workaround for the following bug: ros/geometry2#275
@trainman419
Copy link
Contributor

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
Copy link
Member

tfoote commented Nov 8, 2018

@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
Copy link

mintar commented Nov 9, 2018

👍

we can remove it in a future rosdistro

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

@jspricke
Copy link
Member

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

@trainman419
Copy link
Contributor

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).

jspricke added a commit to jspricke/geometry2 that referenced this issue Jan 25, 2019
@jspricke
Move tf2::impl::toQuaternion to tf2_geometry_msgs
3d08107 
Closes: ros#275

This breaks the cycle between tf2 and tf2_geometry_msgs.
I didn't add deprecated attributes for functions having
tf2_geometry_msgs in the method signature, as they would need to have
the right header imported anyhow.
@jspricke jspricke linked a pull request Jan 25, 2019 that will close this issue
Open
jspricke added a commit to jspricke/geometry2 that referenced this issue Jan 25, 2019
@jspricke
Move tf2::impl::toQuaternion to tf2_geometry_msgs
f2b6318 
Closes: ros#275

This breaks the cycle between tf2 and tf2_geometry_msgs.
I didn't add deprecated attributes for functions having
tf2_geometry_msgs in the method signature, as they would need to have
the right header imported anyhow.
jspricke added a commit to jspricke/geometry2 that referenced this issue Jan 25, 2019
@jspricke
Move tf2::impl::toQuaternion to tf2_geometry_msgs
bebb683 
Closes: ros#275

This breaks the cycle between tf2 and tf2_geometry_msgs.
I didn't add deprecated attributes for functions having
tf2_geometry_msgs in the method signature, as they would need to have
the right header imported anyhow.
pjreed added a commit to pjreed/novatel_gps_driver that referenced this issue Nov 18, 2019
@pjreed
Add dependency on tf2_geometry_msgs
9c9220f 
The tf2 package depends on headers from this but does not actually
list it as a dependency; see ros/geometry2#275
@pjreed pjreed mentioned this issue Nov 18, 2019
Merged
pjreed added a commit to swri-robotics/novatel_gps_driver that referenced this issue Nov 18, 2019
@pjreed
Add dependency on tf2_geometry_msgs (#67)
fb164b1 
The tf2 package depends on headers from this but does not actually
list it as a dependency; see ros/geometry2#275
@stertingen stertingen mentioned this issue Jan 19, 2021
Closed
@hsd-dev hsd-dev mentioned this issue Mar 16, 2021
Merged
seanyen pushed a commit to ms-iot/geometry2 that referenced this issue Mar 22, 2021
@surfertas @clalancette
Add type hints to tf2_ros_py code(ros#275) (ros#315)
d7276d5 
* adding typehints to tf2_ros_py code

Co-authored-by: Chris Lalancette <[email protected]>
@ayrton04
Copy link

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.

@ayrton04 ayrton04 mentioned this issue Jul 13, 2021
Merged
@mikepurvis mikepurvis mentioned this issue Jul 15, 2021
Closed
@tfoote
Copy link
Member

tfoote commented Jul 20, 2021

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
Copy link
Member

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

jspricke added a commit to jspricke/geometry2 that referenced this issue Apr 5, 2022
@jspricke
Move tf2::impl::toQuaternion to tf2_geometry_msgs
15b77ac 
Closes: ros#275

This breaks the cycle between tf2 and tf2_geometry_msgs.
I didn't add deprecated attributes for functions having
tf2_geometry_msgs in the method signature, as they would need to have
the right header imported anyhow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move tf2::impl::toQuaternion to tf2_geometry_msgs jspricke/geometry2
6 participants
@trainman419 @mintar @tfoote @jspricke @Crusty82 @ayrton04