Thanks for the feedback. The CMakeLists.txt should now be very similar (regarding the python parts) to the one you linked. Also, I reactivated the tests, which were previously not included in the build and test process.

Great, thanks for iterating here. I just made some additional updates that should reduce warnings and make things work a little better. At least in my testing on Linux, this is pretty good now.

One thing that I will request is that we make as few changes as possible to the tests. The major reason for that is that the more changes we make here, the harder it is for us to "upgrade" to a newer version of orocos_kinematics_dynamics from upstream. So let's just do the minimum changes necessary to make this work. Once we've done that, I'll run CI on it to see how well this all works on Windows.

Oh, and one more thing: if I run the tests repeatedly, I eventually get a test failure in kinfamtest.py that looks like:

3: self = <kinfamtest.KinfamTestFunctions testMethod=testFkPosAndIkPos>
3: 
3:     def testFkPosAndIkPos(self):
3:         q = JntArray(self.chain.getNrOfJoints())
3:         for i in range(q.rows()):
3:             q[i] = random.uniform(-3.14, 3.14)
3:     
3:         q_init = JntArray(self.chain.getNrOfJoints())
3:         for i in range(q_init.rows()):
3:             q_init[i] = q[i] + 0.1 * random.random()
3:     
3:         q_solved = JntArray(q.rows())
3:     
3:         F1 = Frame.Identity()
3:         F2 = Frame.Identity()
3:     
3:         self.assertTrue(0 == self.fksolverpos.JntToCart(q, F1))
3:         self.assertTrue(0 == self.iksolverpos.CartToJnt(q_init, F1, q_solved))
3:         self.assertTrue(0 == self.fksolverpos.JntToCart(q_solved, F2))
3:     
3:         self.assertEqual(F1, F2)
3: >       self.assertEqual(q, q_solved)
3: E       AssertionError:  -2.70393
3: E         1.56895
3: E       -0.086594
3: E        0.345345
3: E        -2.87941
3: E        -2.51235 !=   -2.70393
3: E          1.56895
3: E       -0.0865901
3: E         0.345346
3: E         -2.87941
3: E         -2.51235

That needs to be looked into, because we want to avoid introducing a flakey test onto the buildfarm.

Okay, good point, cosmetic changes to the tests were reverted now. Some function names I had to change though, because they do not seem to be meant to be called by the test runner directly, but they had "test" in the name, which caused them to be run directly.

Regarding the flaky test you mention, I could reproduce the failure in 2 out of 20 runs. The test is doing forward and inverse kinematics and checks if the joint configuration in the end is the same as in the beginning. However, I think there is no guarantee that this has to be the case if the chain we're looking at is redundant, i.e. in that case there can be multiple joint configurations satisfying a certain end effector configuration. Therefore, I'd say that it is not the implementation of the test that is flaky but the nature of the underlying problem. To remove the "flakiness", I see two options:

1. We could remove the test, i.e. the line self.assertEqual(q, q_solved).
2. Instead of requiring the joint configurations to be equal, we could require them to be close, e.g. norm less than a tolerance.

What do you think?

Okay, good point, cosmetic changes to the tests were reverted now. Some function names I had to change though, because they do not seem to be meant to be called by the test runner directly, but they had "test" in the name, which caused them to be run directly.

Yeah, that's fine. The goal is to minimize the change, not eliminate it completely. What you've done here looks OK to me.

Regarding the flaky test you mention, I could reproduce the failure in 2 out of 20 runs. The test is doing forward and inverse kinematics and checks if the joint configuration in the end is the same as in the beginning. However, I think there is no guarantee that this has to be the case if the chain we're looking at is redundant, i.e. in that case there can be multiple joint configurations satisfying a certain end effector configuration. Therefore, I'd say that it is not the implementation of the test that is flaky but the nature of the underlying problem. To remove the "flakiness", I see two options:

1. We could remove the test, i.e. the line `self.assertEqual(q, q_solved)`.

2. Instead of requiring the joint configurations to be equal, we could require them to be close, e.g. norm less than a tolerance.

I'd rather keep the tests if we can make them work. So I'd say let's check the norm less a tolerance.

Just updated the test to check a tolerance.
Although I tried again to reproduce the failure of the original test and couldn't observe a single failure after running it 100 times, so it seems that this case is very rare. Anyways, now with the tolerance check it should be robust.

With this fixed, are we ready to merge or do you have more feedback?

It looks generally good to me, and I haven't seen a failure in over a 1000 tries locally here. Here's a run on CI to see where we stand (particularly with respect to macOS and Windows):

• Linux
• Linux-aarch64
• macOS
• Windows

So it looks like that frametest is still failing on Linux-aarch64 (maybe a numerical error?). Also, macOS is failing to compile this new package, not being able to find Python.h. Finally, Windows is failing to find SIP at all. We may have to install SIP in the Windows Dockerfile; I'm not sure.

The numerical stability of the test should be improved now, so the build on Linux-aarch64 should succeed.

Regarding the build on mac, it seems that sip.h tries to #include <Python.h> which cannot be found. According to this, on mac, one should #include <Python/Python.h>. SIP seems to be installed through brew on the build server. We could open a discussion on https://github.com/Homebrew/discussions/discussions to ask how to solve this. I guess it's very unlikely that the SIP code can be changed to include Python/Python.h, so instead we probably need to modify/add include directories s.t. Python.h can be found directly. However, I don't know where Python.h lives on the build server, and I don't have access to a mac build environment to test this.

And regarding the build on Windows, I also don't know how to make SIP available there.

How would you suggest to proceed with these two issues?

What's the state of this? It would be really nice to be able to use tf2_geometry_msgs with ROS 2.

What's the state of this? It would be really nice to be able to use tf2_geometry_msgs with ROS 2.

For the record, tf2_geometry_msgs works fine in C++ in ROS 2. What this will allow is using tf2_geometry_msgs from Python, which currently doesn't work.

The latest here was that this was failing quite a bit of CI, as you can see in #19 (comment) . That would need to be fixed to make any progress here.

That said, this whole thing may be obsoleted by the discussions at #24 and ros2/ros2#1208 .

Yes, I am talking about tf2_geometry_msgs for Python, I know that they work for C++.

Thank you for the update and links to the other pull requests. All the pull requests I found were last active in February, but the ones you linked look like the issue is currently being worked on, so I probably just have to be patient for a little more time.

ros2/ros2#1208 proposes switching to vendor packages for orocos_kdl and python_orocos_kdl, instead of continuing to maintain this fork. The vendor packages will check if the packages are already installed on the system and build them from source otherwise (including the Python package). So, I think we can close this PR.

Note, the vendor packages will probably only be released into Rolling (and Humble by extension).

And we've now done the switchover to the vendored orocos package. With that, I'm going to close this out.