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

Make tf2_py compatible with python3. #173

Merged
merged 4 commits into from
Jun 10, 2016
Merged

Make tf2_py compatible with python3. #173

merged 4 commits into from
Jun 10, 2016

Conversation

de-vri-es
Copy link
Contributor

This PR makes tf2_py compatible with python3. I compiled the package for both python 3.5 and 2.7 (which worked), but I did not test the resulting python module.

There were three issues to fix:

  • Python3 uses unicode strings for text. The old python2 string type no longer exists.
  • Module initialization has been overhauled for python3.
  • Type definitions changed slightly.

I put wrapper functions for the correct string types in python_compat.h (and one convenience function for importing modules). Something similar could be done for the type definition and the module initialization, but those would involve writing new macros, which would not necessarily make the code easier to maintain (especially in the case of module initialization).

I did not use py3c (a C API compatibility layer for python2/3) to avoid adding a new dependency. If a new dependency is acceptable, py3c would be a good alternative. It would involve adding a new rosdep key though, I assume.

If this is the way to go, PyObject_BorrowAttrString could also be moved to python_compat.h, to group python convenience functions together. (Though I would rename it so it doesn't look like a real python function. It took me a while of digging through the python C API before realising it is defined in the source file itself).

@de-vri-es
Copy link
Contributor Author

This should fix #50, btw.

@de-vri-es
Copy link
Contributor Author

de-vri-es commented May 6, 2016

One more addendum: Rewriting the whole thing in Cython seems like a good idea to me, perhabs even with a more full-featured API (while maintaining backwards compatibility of course).

However, I wanted python3 compatibility fast, and this was the quickest, least intrusive way to get that.

@tfoote
Copy link
Member

tfoote commented Jun 7, 2016

@ros-pull-request-builder retest this please

Thanks for the contribution. I'll try to review it tomorrow. Python3 support will be great.

@tfoote
Copy link
Member

tfoote commented Jun 9, 2016

Thanks for taking the time to do this. It looks good to me. Bumping PyObject_BorrowAttrString into the python_compat.h makes sense to me.

Thanks for working around it without the extra dependency. If this is enough lets stick with this and not completely refactor it to Cython.

@de-vri-es
Copy link
Contributor Author

de-vri-es commented Jun 10, 2016

Moved PyObject_BorrowAtrrString to python_compat.h and renamed it to pythonBorrowAttrString to avoid looking like it is part of the official Python API.

(Update: And made it inline, since it is defined in a header.)

Thanks for the review.

@tfoote tfoote merged commit ae3dd71 into ros:indigo-devel Jun 10, 2016
@tfoote
Copy link
Member

tfoote commented Jun 10, 2016

Thanks again!

@spaghetti-
Copy link

Thank you for this patch!

@koniarik
Copy link

Hi folks, will there be anytime soon release of geometry2 with this patch?

@nstiurca
Copy link

nstiurca commented Oct 7, 2016

@SquirrelCZE I'm wondering the same thing.

In the meantime, I built from source based on the latest version I just pulled (439e235), but I can't figure out how to actually use TF2 in Python 3. Here's what I did:

mkdir -p overlay_ws/src
cd overlay_ws/src
git clone https://github.com/ros/geometry2.git
catkin_init_workspace
cd ..
rosdep install --from-paths src --os=ubuntu:xenial
catkin_make --use-ninja --cmake-args -DCMAKE_BUILD_TYPE=Release
source devel/setup.bash

Then I tried to use it with

$ python3
Python 3.5.2 (default, Sep 10 2016, 08:21:44) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import tf2_py
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nicu/dbot/overlay_ws/devel/lib/python2.7/dist-packages/tf2_py/__init__.py", line 35, in <module>
    exec(__fh.read())
  File "<string>", line 37, in <module>
ImportError: No module named '_tf2'
>>> 

@nstiurca
Copy link

nstiurca commented Oct 7, 2016

I should say the import error is the same (except for paths) I get when only working with the latest version installed from APT.

@MohamadHalwani
Copy link

@SquirrelCZE I'm wondering the same thing.

In the meantime, I built from source based on the latest version I just pulled (439e235), but I can't figure out how to actually use TF2 in Python 3. Here's what I did:

mkdir -p overlay_ws/src
cd overlay_ws/src
git clone https://github.com/ros/geometry2.git
catkin_init_workspace
cd ..
rosdep install --from-paths src --os=ubuntu:xenial
catkin_make --use-ninja --cmake-args -DCMAKE_BUILD_TYPE=Release
source devel/setup.bash

Then I tried to use it with

$ python3
Python 3.5.2 (default, Sep 10 2016, 08:21:44) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import tf2_py
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nicu/dbot/overlay_ws/devel/lib/python2.7/dist-packages/tf2_py/__init__.py", line 35, in <module>
    exec(__fh.read())
  File "<string>", line 37, in <module>
ImportError: No module named '_tf2'
>>> 

Have you solved this ??? Coming from the future :D

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

6 participants