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

Used keys for Orocos #38

Merged
merged 3 commits into from
Apr 13, 2020
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Apr 3, 2020

With this PR https://github.com/ros/rosdistro/pull/23841/files Orokos could be installed via rosdep

@ahcorde ahcorde requested a review from sloretz April 3, 2020 09:22
@ahcorde ahcorde self-assigned this Apr 3, 2020
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2020

Maybe for this changes we need to target noetic-devel branch. Because this keys are not availables for bionic

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2020

@ros-pull-request-builder retest this please

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2020

this error has appeared. is the python3-pykdl working fine?

15:38:46 [ROSUNIT] Outputting test results to /tmp/ws/test_results/kdl_parser_py/rostest-test_test_kdl_parser.xml
15:38:46 python: /build/orocos-kdl-mPkyII/orocos-kdl-1.4.0/python_orocos_kdl/PyKDL/std_string.sip:52: int convertTo_std_string(PyObject*, void**, int*, PyObject*): Assertion `PyUnicode_Check(s)' failed.

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 6, 2020

Maybe is this the issue orocos/orocos_kinematics_dynamics#229 ?

@@ -30,10 +30,10 @@
<buildtool_depend condition="$ROS_PYTHON_VERSION == 3">python3-catkin-pkg</buildtool_depend>

<build_export_depend>urdfdom_py</build_export_depend>
<build_export_depend>python_orocos_kdl</build_export_depend>
<build_export_depend>python3-pykdl</build_export_depend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind deleting this line? It doesn't look like a <build_export_depend> is necessary; the <exec_depend> below should be enough.

@sloretz sloretz changed the base branch from melodic-devel to noetic-devel April 8, 2020 15:45
@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

Retargeted at noetic-devel since melodic should keep using ros packages.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

Build Status

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

this error has appeared. is the python3-pykdl working fine?

Investigating, minimal reproducible example:

import urdf_parser_py.urdf as urdf
import kdl_parser_py.urdf

xml = """<?xml version="1.0"?>
<robot name="physics">
  <link name="base_link"/>
</robot>
"""

model = urdf.URDF.from_xml_string(xml)
(ok, tree) = kdl_parser_py.urdf.treeFromUrdfModel(model)

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

Even smaller example:

$ python3 -c "import PyKDL; PyKDL.Tree('foobar')"
python3: /build/orocos-kdl-mPkyII/orocos-kdl-1.4.0/python_orocos_kdl/PyKDL/std_string.sip:52: int convertTo_std_string(PyObject*, void**, int*, PyObject*): Assertion `PyUnicode_Check(s)' failed.
Aborted (core dumped)

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

Another user encountered same bug: orocos/orocos_kinematics_dynamics#59 (comment)

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 8, 2020

I was able to remove the error with this branch orocos/orocos_kinematics_dynamics#229

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

I was able to remove the error with this branch orocos/orocos_kinematics_dynamics#229

Since we're using python3-pykdl from upstream, we'll need to get it fixed in Debian Buster and Ubuntu Focal. I opened bugs upstream.

In the meantime, maybe we can workaround the issue by avoiding any KDL api's that use strings?

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

Avoid APIs that cause KDL to crash in 80b0918

sloretz
sloretz previously approved these changes Apr 8, 2020
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clalancette mind checking if the workaround looks ok? I don't see any packages that depend on kdl_parser_py directly (besides the robot metapackage), so I'm not sure what we'll break by not naming things.

@sloretz sloretz dismissed their stale review April 8, 2020 23:49

Ooops, must have missed something; same failure in CI

@sloretz
Copy link
Contributor

sloretz commented Apr 8, 2020

It seems there's no way to avoid the string APIs, at least I can't avoid addSegment() :( . This will need to wait for the bug to be fixed upstream.

now exiting InteractiveConsole...
Traceback (most recent call last):
  File "mre.py", line 241, in <module>
    (ok, tree) = kdl_parser_py.urdf.treeFromUrdfModel(model)
  File "/home/sloretz/ws/noetic/src/kdl_parser/kdl_parser_py/kdl_parser_py/urdf.py", line 130, in treeFromUrdfModel
    if not _add_children_to_tree(robot_model, robot_model.link_map[child], tree):
  File "/home/sloretz/ws/noetic/src/kdl_parser/kdl_parser_py/kdl_parser_py/urdf.py", line 96, in _add_children_to_tree
    if not tree.addSegment(sgm):
TypeError: addSegment(self, segment: Segment, hook_name: object): not enough arguments
[Inferior 1 (process 17992) exited with code 01]

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I added a CATKIN_IGNORE to kdl_parser_py for now. This enables releasing to unblock robot_state_publisher. Hopefully the python3-pykdl fixes make it upstream before it's time to release metapackages since the robot package depends on kdl_parser_py.

@sloretz sloretz merged commit a33bf97 into ros:noetic-devel Apr 13, 2020
@sloretz sloretz mentioned this pull request Aug 24, 2020
MatthijsBurgh added a commit to tue-robotics/kdl_parser that referenced this pull request Feb 19, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants