ros-drivers / openni2_camera

ROS wrapper for openni 2.0
http://wiki.ros.org/openni2_camera
BSD 3-Clause "New" or "Revised" License
56 stars 96 forks source link

make subprocess output a string instead of bytes, needed in python 3.7 #113

Closed lucasw closed 11 months ago

lucasw commented 2 years ago

Running roswtf resulted in this exception on Ubuntu 20.04 and ros noetic both in the apt released version and current source here:

running tf checks, this will take a second...
... tf checks complete
Traceback (most recent call last):
  File "<string>", line 224, in _roswtf_main
  File "/opt/ros/noetic/lib/python3/dist-packages/openni2_launch/wtf_plugin.py", line 114, in roswtf_plugin_online
    error_rule(r, r[0](ctx), ctx)
  File "/opt/ros/noetic/lib/python3/dist-packages/openni2_launch/wtf_plugin.py", line 85, in sensor_notfound
    devices = _device_notfound_subproc(
  File "/opt/ros/noetic/lib/python3/dist-packages/openni2_launch/wtf_plugin.py", line 58, in _device_notfound_subproc
    for i in df.split('\n'):
TypeError: a bytes-like object is required, not 'str'
a bytes-like object is required, not 'str'

Adding text=True to the subprocess call looks to be the correct thing to do in newer python versions:

https://docs.python.org/3/library/subprocess.html#subprocess.CompletedProcess.stdout

https://stackoverflow.com/questions/5902485/can-i-have-subprocess-call-write-the-output-of-the-call-to-a-string

130s commented 1 year ago

Thank you for the contribution.

Although I won't have a room to take a closer look nor test myself, I'm inclined to merging (and depend on the OSS community to test). In a few days I might merge unless we'll receive objections.

adivardi commented 11 months ago

@130s sorry to at you, but since it is approved already it would be nice to merge. This PR fixes the package for python3 and is blocking any usage of roswtf with this package installed.

130s commented 11 months ago

Thanks for the headsup. Merged, made 1.6.1 release for ros1 branch https://github.com/ros-drivers/openni2_camera/pull/133, and made a request to build an installer package for ROS Noetic https://github.com/ros/rosdistro/pull/38806.