-
Notifications
You must be signed in to change notification settings - Fork 198
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
Split tf2_kdl and Port Python Functionality #665
base: rolling
Are you sure you want to change the base?
Conversation
tf2_kdl_py/package.xml
Outdated
|
||
<test_depend>python3-pytest</test_depend> | ||
<test_depend>rclpy</test_depend> | ||
<test_depend>tf2_msgs</test_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see this used in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple things that I think we should fix. Otherwise, this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left one more change inline.
Besides that, I realize that we should also update https://github.com/ros2/geometry2/blob/rolling/geometry2/package.xml . We haven't been totally consistent here, but I think we should add in tf2_kdl_py
as well as tf2_ros_py
as dependencies there.
And finally, I realize that the way that this is installed makes it so that import tf2_kdl
no longer works (import tf2_kdl_py
works, but we need to maintain backwards compatibility). This becomes obvious once you make the changes to make the tests work (inline). We need to fix that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some things to do:
- I can see the tf2_kdl CMakeLists.txt that you are installing the python package
- Remove this comment from the CMakeLists.txt
- As this PR splits the cpp and python implementation this dependency in the package.xml is not required
- Finally you can remove this file
src/tf2_kdl
@CursedRock17 do you mind to merge with |
Signed-off-by: CursedRock17 <[email protected]> Removing extra dependencies Signed-off-by: CursedRock17 <[email protected]> Updating package version Signed-off-by: CursedRock17 <[email protected]> Correcting dependencies Signed-off-by: CursedRock17 <[email protected]> Fixing imports Signed-off-by: CursedRock17 <[email protected]> Removing excess from tf2_kdl Signed-off-by: CursedRock17 <[email protected]> Fixing imports Signed-off-by: CursedRock17 <[email protected]> Removing excess from tf2_kdl Signed-off-by: CursedRock17 <[email protected]>
@ahcorde it was rebased locally with rolling, so it should be good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://ci.ros2.org/job/ci_linux-aarch64/15037/console
09:59:55 ImportError while importing test module '/home/jenkins-agent/workspace/ci_linux-aarch64/ws/src/ros2/geometry2/tf2_kdl_py/test/test_kdl.py'.
09:59:55 Hint: make sure your test modules/packages have valid Python names.
09:59:55 Traceback:
09:59:55 /usr/lib/python3.10/importlib/__init__.py:126: in import_module
09:59:55 return _bootstrap._gcd_import(name[level:], package, level)
09:59:55 test/test_kdl.py:37: in <module>
09:59:55 import tf2_kdl # noqa(F401)
09:59:55 E ModuleNotFoundError: No module named 'tf2_kdl'
Signed-off-by: CursedRock17 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests are failing on Linux and there is a problem with PyKDL on Windows
Maybe the error on Windows is because PyKDL is not installed on Windows. @clalancette ? |
We build it from source there: https://ci.ros2.org/job/ci_windows/21434/consoleFull#console-section-182 |
I might be mistaken, but I feel like the CI will continuously fail and show warnings until #110 is resolved, that being said I'm working on a port right now. Would it be more beneficial to comment out the |
So I guess I'm confused; isn't the point of this PR to resolve #110? I guess if not, then what we should do is to both solve #110 and #208 in this PR simultaneously. Otherwise, there is no point to having a split |
Gotcha, the original plan was resolving just #208 but since I'm already prepping a port, I can add it to this PR. It makes it much easier this way anyway. |
I was checking this on a Windows machine, the problem is on |
Signed-off-by: CursedRock17 <[email protected]>
So it's been a while, but I've got a valid product in which the Python portion of KDL has been ported to match the C++ features. I still have not gotten the chance to change the path on windows, but I'm more concerned with a tier one solution for the mean time. Changes so far:
|
This is meant to resolve #208 by creating a
tf2_kdl_py
package and moving all the python parts fromtf2_kdl
to it.The PR will be good preparation for #110 as PyKDL is now available.