-
Notifications
You must be signed in to change notification settings - Fork 10
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
port python_orocos_kdl to ros2 #4
Conversation
@@ -39,7 +39,7 @@ | |||
// If argument is a Python string, assume it's UTF-8 | |||
if (sipIsErr == NULL) | |||
#if PY_MAJOR_VERSION < 3 | |||
return (PyString_Check(sipPy) || PyUnicode_Check(sipPy)); | |||
return (PyBytes_Check(sipPy) || PyUnicode_Check(sipPy)); |
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.
It looks like the PyBytes_X
functions are Python3 only (https://docs.python.org/2.7/c-api/string.html) so the PyString_X
calls within the #if PY_MAJOR_VERSION < 3
should stay unchanged AFAICT
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.
Seems right. As stated in the PR, changes within the SIPs are by @rkojcev, but if needed I can rebase and remove those changes.
if (PyString_Check(sipPy)) { | ||
*sipCppPtr = new std::string(PyString_AS_STRING(sipPy)); | ||
if (PyBytes_Check(sipPy)) { | ||
*sipCppPtr = new std::string(PyBytes_AS_STRING(sipPy)); |
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.
As above, this is within a #if PY_MAJOR_VERSION < 3
block so probably it can be left untouched.
I've removed the unnecessary changes and added one additional python2/python3 check so that it should work with both. |
f0e3f64
to
7ef822c
Compare
thanks @mjbogusz for iterating. Do you have any reference code you use for testing the good functionment of this package ? (as it doesn't have tests) |
Currently I do not, I'm yet to create my modules utilizing PyKDL (as of now I'm only creating some frames and multiply them together - this much seems to work). However if you'd point me towards a good example of how to set up tests in ament_cmake I can try to run the ones included in |
I've fixed the included tests ( I'm working on including them in Edit: I've just noticed that those tests use |
Tests are in 👍 |
Thanks @mjbogusz !
Yeah ament_python has some macros that go in that direction but nothing that would work out of the box for sip packages AFAICT.
👍 I don't have write access to this branch so I pushed a few fixups to https://github.com/ros2/orocos_kinematics_dynamics/tree/ros2-pykdl (mostly to get the tests to run without having to source the workspace). Would you mind cherry-picking them on this branch ? I think with the fixups it's good to be merged 👍 Looks like just installing sip was not enough to get it to build on our farm: I'll take a closer look at it early next week |
You mean, merging them into my PR branch? |
Yes please 👍 , any solution bringing the changes from 20b998a into this PR branch is fine |
Done Edit: I've also added a status message in |
…n ROS 2, fix Python3 compilation with @vmayoral
As we are using Ubuntu Xenial (python3.5) the error would be ImportError (ModuleNotFoundError is a 3.6 feature AFAICT). If you look at the OSX and Windows jobs that failed above (they use 3.6.3) they do raise I looked a bit into it and I it looks like its using the right python interpreter its because of a mismatch between the sip modules installed when installing The one from apt ( |
I've run this file on some configurations. Seems just about right 👌 maybe except the doubled output when sipconfig is not available ;) However while I'm not sure if that's an issue here, SIP's pip page says:
Anyway, here are outputs from different configs: Arch with python 3.6.3 and SIP installed via pacman (
WSL (aka Bash on Windows - here acting as Xenial) with python 3.5.2 and SIP from pip3:
Same WSL with SIP from apt (
Also it's quite interesting what files are inside the packages available for Xenial:
|
Also while scrolling through jenkins logs I've found out that it installs SIP from both apt (while building dockerfile, at 14:34:05 of this specific run) and from pip3 (while installing python packages within the container in venv, 14:40:48, 14:40:54 and 14:40:55) |
Haha yeah its just me printing my debug output to stderr. That's not going to stay :)
Yes you're right, sorry I should have pointed it out earlier, we currently do the same venv setup for all platforms (including linux regardless of what has been installed via apt in the Dockerfile), that's definitely something we could iterate on the later. The main thing I wanted to confirm is "is installing sip from pip is enough to build this code?". As you saw in the release notes and as I confirmed locally: the answer is no (we need the sip headers, libraries and generator for this package to build). I confirmed that building sip from source and installing it provides all the necessary files so tweaking the FindSIP.py script will become irrelevant (as the from source install does provide the So what we need is a way to get the full sip on all platforms:
|
I'd say that a chocolatey package is better - both vendor packages you've linked are working OK, but some are not as good, e.g. rviz_yaml_cpp_vendor (ros2/rviz#152 and related PRs). Also I think it'll save some time when RQT tools will start getting ported to ROS2 - I may be mistaken but I think PyQT required SIP. |
You're totally right, I actually think that SIP was developed by the PyQt effort.
Yeah in the case of the ones I linked, they provide a failsafe as if users don't have the dep on their system it will download and install it for them. So they work "in addition to" the chocolatey packages. I'll test the MacOS test case on Monday. If it works out of the box:
If it doesnt work out of the box and seems painful:
How does that sound ? |
Sounds A-OK. I've tried looking into creating a choco package, but I've spent way too much time setting up the whole required VC++ stack. Also I have no idea how to install python modules using chocolatey - assuming e.g. |
So it looks like python modules installed via brew are not findable in a python virtualenv, they recommend using pip instead. As the sip version doesnt provide us the files we need we'll have to do a vendor package for now.
That;s a good question, we haven't packages any mixed python/C++ packages with choco yet. Maybe @nuclearsandwich will have some insights on the preferred install locations |
It should work if one was to believe the docs... And as per https://stackoverflow.com/a/26322102/3217805 (the 'edit' part) it might be required to manually set
However I have no experience with brew (or development on osx) whatsoever so that's just a About choco: I've found out that Also, if I understand it well enough, @if not "%WindowsSdkDir%" == "" @set PATH=%WindowsSdkDir%bin\x86;%PATH%
@if not "%WindowsSdkDir%" == "" @set INCLUDE=%WindowsSdkDir%include\%WindowsSDKVersion%shared;%WindowsSdkDir%include\%WindowsSDKVersion%um;%WindowsSdkDir%include\%WindowsSDKVersion%winrt;%INCLUDE%
@if not "%WindowsSdkDir%" == "" @set LIB=%WindowsSdkDir%lib\%WindowsSDKLibVersion%um\x86;%LIB%
@if not "%WindowsSdkDir%" == "" @set LIBPATH=%WindowsLibPath%;%ExtensionSDKDir%\Microsoft.VCLibs\14.0\References\CommonConfiguration\neutral;%LIBPATH%
:: and then
@if exist "%VCINSTALLDIR%ATLMFC\INCLUDE" set INCLUDE=%VCINSTALLDIR%ATLMFC\INCLUDE;%INCLUDE%
@if exist "%VCINSTALLDIR%INCLUDE" set INCLUDE=%VCINSTALLDIR%INCLUDE;%INCLUDE% |
A colleague of mine has tried using this package in Ubuntu 17.10 and 16.04 and he was consistently encountering a segmentation fault when requiring PyKDL (stacktrace pointed at missing As a sidenote, any news on the vendor package? |
@clalancette is anyone working on this at the moment? |
Not that I know of; @mjbogusz may still be interested. I think there is the open question about SIP, but it is worth looking at again. |
Closing due to long time inactivity._ |
Any updates on this? |
This PR enables PyKDL usage in ROS2.
Fixes in SIP are by @rkojcev and @vmayoral, so is basic adaptation of
package.xml
andCMakeLists.txt
to ament, I've additionally added triggeringPYTHONPATH
hook (as ament does not support SIP packages yet) and improved installation of compiled objects.