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

towards python3 support #1054

Merged
merged 5 commits into from Sep 19, 2018

Conversation

Projects
None yet
4 participants
@v4hn
Copy link
Member

v4hn commented Sep 8, 2018

This series of (hopefully backward-compatible) patches enables us to build MoveIt with python 3 if the underlying ROS distribution was compiled with python 3 too.

It also fixes a number of common python 2/3 compatibility issues.

Additionally it supports the new boost-style for the python component.

With this I can successfully build MoveIt in python3 and run the moveit_commander_cmdline.py to get a prompt.
There is still open issues (if you try to "use" a group, the shell dies...), but I will leave those to someone else.

@timonegk

This comment has been minimized.

Copy link
Contributor

timonegk commented Sep 8, 2018

VERSION_GREATER_EQUAL was introduced in cmake 3.7. The minimum required version here is 2.8.12. That is why the CI fails. You can use if(NOT (Boost_VERSION LESS 106700)) instead. (Or swap if and else and drop the NOT.)

@felixvd

This comment has been minimized.

Copy link
Contributor

felixvd commented Sep 9, 2018

If you write out the open issues and how to compile with python3, maybe someone could pursue this on WMD?

@davetcoleman davetcoleman requested a review from mlautman Sep 11, 2018

@v4hn v4hn force-pushed the v4hn:pr-kinetic-python3 branch from 60f7b08 to 6b51d72 Sep 11, 2018

@v4hn

This comment has been minimized.

Copy link
Member Author

v4hn commented Sep 11, 2018

Thanks for looking up the minimum version @timonegk .
I didn't think (1) the VERSION conditionals were that new and (2) ubuntu xenial only provides cmake 3.5.
The problem is not so much the minimum_required version but the one installed on the system...

I changed it as you suggested.

@felixvd That was the plan, feel free to look into it. :)
It's quite a bit of compile time/work though to setup a full ROS python3 ROS workspace,
so as a WMD task this has to be somewhat prepared I guess.

@mlautman
Copy link
Member

mlautman left a comment

+1 after fixing __biltin__ vs __builtins__

@@ -10,6 +12,11 @@
import argparse
from moveit_commander import MoveGroupCommandInterpreter, MoveGroupInfoLevel, roscpp_initialize, roscpp_shutdown

# compatibility with python3
# python2's input function is dangerous anyway
if hasattr(__builtins__, 'raw_input'):

This comment has been minimized.

@mlautman

v4hn added some commits Sep 8, 2018

detect correct version of python/boost-python
There is no need to hardcode python2 in here anymore.
Instead rely on whatever ROS was configured to use.

https://www.boost.org/doc/libs/1_68_0/libs/python/doc/html/rn.html#rn.version_1_67
Additionally current releases of boost use python27/python36 for the component.
moveit_commander: fix many implicit local includes
Python3 requires these to be explicit.

@v4hn v4hn force-pushed the v4hn:pr-kinetic-python3 branch from 6b51d72 to 8e0e087 Sep 16, 2018

@v4hn

This comment has been minimized.

Copy link
Member Author

v4hn commented Sep 16, 2018

@mlautman mlautman merged commit 8c62cf2 into ros-planning:kinetic-devel Sep 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

v4hn added a commit that referenced this pull request Sep 23, 2018

towards python3 support (#1054)
* detect correct version of python/boost-python

There is no need to hardcode python2 in here anymore.
Instead rely on whatever ROS was configured to use.

https://www.boost.org/doc/libs/1_68_0/libs/python/doc/html/rn.html#rn.version_1_67
Additionally current releases of boost use python27/python36 for the component.

* moveitjoy: fix single print statement

* moveit_commander: fix print function usage

* moveit_commander: fix many implicit local includes

Python3 requires these to be explicit.

* moveit_commander: python3-compatible cli input

v4hn pushed a commit to v4hn/moveit that referenced this pull request Sep 27, 2018

TAMS demo laptop
Revert "towards python3 support (ros-planning#1054)"
This reverts commit 8c62cf2.

Breaks Python2 support

mlautman added a commit to PickNikRobotics/moveit that referenced this pull request Oct 8, 2018

towards python3 support (ros-planning#1054)
* detect correct version of python/boost-python

There is no need to hardcode python2 in here anymore.
Instead rely on whatever ROS was configured to use.

https://www.boost.org/doc/libs/1_68_0/libs/python/doc/html/rn.html#rn.version_1_67
Additionally current releases of boost use python27/python36 for the component.

* moveitjoy: fix single print statement

* moveit_commander: fix print function usage

* moveit_commander: fix many implicit local includes

Python3 requires these to be explicit.

* moveit_commander: python3-compatible cli input

@rhaschke rhaschke referenced this pull request Oct 20, 2018

Merged

Fixed moveit_commander __builtin__ missing error #1103

2 of 7 tasks complete

rhaschke added a commit that referenced this pull request Oct 21, 2018

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.