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

[Noetic] changes to make it work with Python3 #1069

Merged
merged 14 commits into from
May 14, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 26, 2020

Changes to make it work with Python3

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 31, 2020

For some reason when the second test starts in gazebo_ros called ros_network/ros_network_disabled.test Gazebo dies immediately. The test fails with a timeout failure.

I tested each one of them individually, and it works. any thoughts?

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 31, 2020

Chnged enviroment variable GAZEBO_MASTER_URI to avoid the issue the collision of gazebo instances because of the binded ports

Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

A few minor comments. Should the Python files have python3 in the shebang or are they intended to still support python 2?

gazebo_ros tests worked for me. I didn't run the gazebo_plugins tests (that's problem on my end. I can't get its dependencies to compile yet...)

gazebo_plugins/scripts/gazebo_model Show resolved Hide resolved
gazebo_plugins/test/bumper_test/test_bumper.py Outdated Show resolved Hide resolved
gazebo_plugins/test/p3d_test/test_link_pose.py Outdated Show resolved Hide resolved
ahcorde and others added 3 commits April 22, 2020 08:51
Co-Authored-By: Mabel Zhang <mabel.m.zhang@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 22, 2020

shebang lines look like : #! /usr/bin/env python. Ubuntu Focal only allows Python3 which I think it's fine with the python version installed in the system

@mabelzhang
Copy link
Contributor

I think for Python3, usually the shebang lines would look like #!/usr/bin/env python3.

@sloretz
Copy link
Contributor

sloretz commented Apr 23, 2020

A few minor comments. Should the Python files have python3 in the shebang or are they intended to still support python 2?

@mabelzhang @ahcorde IIUC PEP 394 recommends shebangs match the python version (python2 or python3) when they're installed. There's a CMake macro catkin_install_python() that writes the shebang to the specific python version used. The current recommendation is to use #!/usr/bin/env python and catkin_install_python() http://wiki.ros.org/UsingPython3/SourceCodeChanges#Changing_shebangs . Is that macro being used here?

@mabelzhang
Copy link
Contributor

Thank you for the clarification and the link!
Looks like the current CMakeLists.txt for gazebo_plugins and gazebo_ros do not have catkin_install_python()
https://github.com/ros-simulation/gazebo_ros_pkgs/blob/noetic-devel/gazebo_plugins/CMakeLists.txt
https://github.com/ros-simulation/gazebo_ros_pkgs/blob/noetic-devel/gazebo_ros/CMakeLists.txt

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 24, 2020

the scripts in gazebo_ros are written in bash not in Python. Should I install them with catkin_install_python()?

@sloretz
Copy link
Contributor

sloretz commented Apr 24, 2020

the scripts in gazebo_ros are written in bash not in Python. Should I install them with catkin_install_python()?

Definitely not. If they're bash scripts, what has the #!/usr/bin/env python shebang?

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 24, 2020

gazebo_ros has some, but right now it's fixed 6cabcc7

gazebo_plugins/scripts/gazebo_model Outdated Show resolved Hide resolved
gazebo_plugins/scripts/set_pose.py Outdated Show resolved Hide resolved
gazebo_plugins/scripts/set_wrench.py Outdated Show resolved Hide resolved
gazebo_ros/test/ros_network/ros_api_checker Outdated Show resolved Hide resolved
gazebo_ros/test/ros_network/ros_api_checker Outdated Show resolved Hide resolved
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM! Tests pass on my machine using Gazebo 11 (currently using gazebo11 from packages.osrfoundation.org, but hope to use packages.ros.org when that becomes available).

@sloretz
Copy link
Contributor

sloretz commented May 14, 2020

@chapulina @j-rivero I think this should go in before the other Noetic PRs since it has changes required for Noetic. Mind taking a look, and if it looks ok then merging it?

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me. Let's just merge it like this, but beware that the *11 rosdep keys are not working yet, see ros-infrastructure/reprepro-updater#75 (comment).

@chapulina chapulina merged commit c205dd7 into ros-simulation:noetic-devel May 14, 2020
seanyen pushed a commit to ms-iot/gazebo_ros_pkgs that referenced this pull request Aug 31, 2020
* Noetic - changes to make it work with Python3

Signed-off-by: ahcorde <ahcorde@gmail.com>
Co-authored-by: Mabel Zhang <mabel.m.zhang@gmail.com>
Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
cohen39 pushed a commit to cohen39/gazebo_ros_pkgs that referenced this pull request Nov 15, 2021
* Noetic - changes to make it work with Python3

Signed-off-by: ahcorde <ahcorde@gmail.com>
Co-authored-by: Mabel Zhang <mabel.m.zhang@gmail.com>
Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
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.

4 participants