8000 roslz4: Improve py3k compatibility. by aballier · Pull Request #563 · ros/ros_comm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

roslz4: Improve py3k compatibility. #563

Merged
merged 1 commit into from
Feb 4, 2015
Merged

roslz4: Improve py3k compatibility. #563

merged 1 commit into from
Feb 4, 2015

Conversation

aballier
Copy link
Contributor
@aballier aballier commented Feb 2, 2015

Use explicit relative import, py3.3 and 3.4 refuse to import roslz4 otherwise.

This is trivial and backward compatible, but unfortunately very far from being sufficient for py3k support. I'm filling this to avoid letting it rot in my local tree.

@aballier
roslz4: Improve py3k compatibility.
ccc63de 
Use explicit relative import, py3.3 and 3.4 refuse to import roslz4 otherwise.
@ros-pull-request-builder
Copy link
Member

Can one of the admins verify this patch?

@dirk-thomas
Copy link
Member

Thanks, the modified import is indeed necessary.

@bcharrow Could you look into making the shared library also importable with Python 3?

@bcharrow
Copy link
Contributor
8000
bcharrow commented Feb 2, 2015

Thanks for the fix! I'll take a look at improving Python 3 compatibility soon.

@aballier
Copy link
Contributor Author
aballier commented Feb 3, 2015

Note: When I was talking about "very far from being sufficient for py3k support" I was talking about the whole ros_comm: I have like half the testsuite failing with it.
roslz4, on the other hand, seems perfectly ok after this patch.

@dirk-thomas
Copy link
Member

The import fails for me due to a missing symbol:

ImportError: dynamic module does not define init function (PyInit__roslz4)

@aballier Are you sure that you have setup the Python 3 environment correctly? Especially you either need a virtual env with all Python packages installed for Python 3 or the python3-* Debian packages (which are not side-by-side installable with the Python 2 ones).

Most of ROS core should work perfectly fine nowadays - as well as most unit tests (with some issue left though).

@aballier
Copy link
Contributor Author
aballier commented Feb 4, 2015

You're probably building with python2 headers: See https://github.com/ros/ros_comm/blob/indigo-devel/utilities/roslz4/src/_roslz4module.c#L411 ; that symbol is defined but under #if PY_MAJOR_VERSION >= 3

I'm not using debian nor virtualenv but gentoo where all sane python-x.y can be installed in parallel (if you think about it, this can quickly be a complete bloat but it is very useful for development and python porting); apart from setting python to the correct version, I also feed cmake with '-DPYTHON_EXECUTABLE="path to versionned python executable"' so maybe that's the reason it finds python2 headers for you.

As for the tests, "half the testsuite" was a bit exagerated :) I had much more failures without the above patch, now it is down to 8/129 failures. It's possible I'm just frightened but that kind of failures might be hard to fix:

ERROR: test_std_msgs_MultiArray (unit.test_genmsg_py.TestGenmsgPy)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.3/site-packages/std_msgs/msg/_UInt8MultiArray.py", line 115, in serialize
    buff.write(struct.pack('<I%ss'%length, length, _x))
struct.error: argument for 's' must be a bytes object

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/var/tmp/portage/dev-ros/ros_comm-1.11.10/work/ros_comm-1.11.10/test/test_rospy/test/unit/test_genmsg_py.py", line 356, in test_std_msgs_MultiArray
    UInt8MultiArray().serialize(buff)
  File "/usr/lib64/python3.3/site-packages/std_msgs/msg/_UInt8MultiArray.py", line 116, in serialize
    except struct.error as se: self._check_types(struct.error("%s: '%s' when writing '%s'" % (type(se), str(se), str(_x))))
  File "/usr/lib64/python3.3/site-packages/genpy/message.py", line 333, in _check_types
    raise SerializationError(str(exc))
genpy.message.SerializationError: <class 'struct.error'>: 'argument for 's' must be a bytes object' when writing ''

======================================================================
ERROR: test_std_msgs_SignedInt (unit.test_genmsg_py.TestGenmsgPy)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/dev-ros/ros_comm-1.11.10/work/ros_comm-1.11.10/test/test_rospy/test/unit/test_genmsg_py.py", line 266, in test_std_msgs_SignedInt
    maxp = long(math.pow(2, w-1)) - 1
NameError: global name 'long' is not defined

@dirk-thomas
Copy link
Member

You are right, I didn't switch to a full Python 3 build. I guess this PR is good to be merged then.

Most of the remaining Python 3 issues are related to ros/genpy#26. But we decided to not spend more time on looking into it for now.

@dirk-thomas
Copy link
Member

@ros-pull-request-builder

@ros-pull-request-builder
Copy link
Member

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/159/

dirk-thomas added a commit that referenced this pull request Feb 4, 2015
@dirk-thomas
Merge pull request #563 from aballier/roslz4_py3k
9da2944 
roslz4: Improve py3k compatibility.
@dirk-thomas dirk-thomas merged commit 9da2944 into ros:indigo-devel Feb 4, 2015
rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017
@dirk-thomas
Merge pull request ros#563 from aballier/roslz4_py3k
5bdfed8 
roslz4: Improve py3k compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aballier @ros-pull-request-builder @dirk-thomas @bcharrow
0