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

Fix rosbag API for Python 3 #1048

Closed

Conversation

nstiurca
Copy link

@nstiurca nstiurca commented May 8, 2017

Closes #1047.

It would be great to get this backported to Kinetic as well.

@dirk-thomas
Copy link
Member

Please provide a reproducible example which failed before / works with the patch.

@nstiurca
Copy link
Author

nstiurca commented May 8, 2017

#!/usr/bin/env python3
import rosbag

with rosbag.Bag('any-bagfile.bag', 'r') as bag:
    for topic, msg, t in bag.read_messages():
        pass

crashes with

Traceback (most recent call last):
  File "./foo.py", line 5, in <module>
    for topic, msg, t in bag.read_messages():
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/rosbag/bag.py", line 2331, in read_messages
    yield self.seek_and_read_message_data_record((entry.chunk_pos, entry.offset), raw)
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/rosbag/bag.py", line 2440, in seek_and_read_message_data_record
    raise ROSBagException('unsupported compression type: %s' % chunk_header.compression)
rosbag.bag.ROSBagException: unsupported compression type: b'none'

Or just run the test suite with Python 3.

@dirk-thomas
Copy link
Member

Currently some of the test are failing. I will try to rerun them (in case they are flaky). @ros-pull-request-builder retest this please

If that doesn't resolve them please look into why they started to fail.

@nstiurca
Copy link
Author

nstiurca commented May 8, 2017 via email

@nstiurca
Copy link
Author

nstiurca commented May 9, 2017

This patch only addresses reading bag files. The test code first creates a bag file, and I found today that there might be some Python 3 related issues there as well. I will update on this as I can.

Although I presume the failed tests above were run using Python 2, so I'm still not sure about that.

@dirk-thomas
Copy link
Member

Although I presume the failed tests above were run using Python 2

Yes, the CI builds only use Python 2.

@@ -1580,6 +1580,7 @@ def _read_uint32(f): return _unpack_uint32(f.read(4))
def _read_uint64(f): return _unpack_uint64(f.read(8))
def _read_time (f): return _unpack_time (f.read(8))

def _unpack_str(v): return v.decode()
Copy link
Member

Choose a reason for hiding this comment

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

The function name doesn't make much sense in the case of Python 3. Since you invoke decode on v it can't be a str but must be bytes?

Matthias Horne and others added 2 commits August 23, 2017 09:44
@nstiurca
Copy link
Author

So it looks like the build is still failing, but if I'm reading the logs right it's an issue with rosdep having timed out during the build rather than anything to do with this patch. Is there a way to re-run the build?

@dirk-thomas
Copy link
Member

I accidentally assumed that #1150 would address the remaining comment on this PR but relied on other parts of the patch. Now I see that the new PR is "complete" on its own. Therefore I have merged #1150 and will close this as a duplicate. Anyway thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants