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

px4-dev: Try to move to Python 3 #199

Merged
merged 3 commits into from
Nov 19, 2019
Merged

px4-dev: Try to move to Python 3 #199

merged 3 commits into from
Nov 19, 2019

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Aug 26, 2019

This goes with PX4/PX4-Autopilot#12809.

@julianoes julianoes requested a review from dagar August 26, 2019 13:08
@TSC21
Copy link
Member

TSC21 commented Aug 26, 2019

This will become problematic for the ROS containers.

@julianoes
Copy link
Contributor Author

So how can we get ROS to Python 3? It's 2019...

@lamping7
Copy link
Member

So how can we get ROS to Python 3? It's 2019...

You can't (without a major effort). Just because it's EOL, doesn't mean it's obsolete.

@julianoes
Copy link
Contributor Author

Alright but as @dagar suggested we can probably have Python 2 dependencies on higher level containers for ROS.

@cclauss
Copy link

cclauss commented Sep 21, 2019

In this repo, these Python 3 syntax errors should be fixed:
flake8 testing of https://github.com/PX4/containers on Python 3.7.1

$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

./docker/ros-indigo/sitl-testing/scripts/testing/export_charts.py:145:44: E999 SyntaxError: invalid syntax
            print "Exporting chart %s to %s" % (chart["name"], path)
                                           ^
./docker/ros-indigo/mavros/tools/broadcaster.py:46:21: E999 SyntaxError: invalid syntax
            print ":".join("{:02x}".format(ord(c)) for c in buf)
                    ^
2     E999 SyntaxError: invalid syntax
2

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. These 5 are different from most other flake8 issues which are merely "style violations" -- useful for readability but they do not effect runtime safety.

  • F821: undefined name name
  • F822: undefined name name in __all__
  • F823: local variable name referenced before assignment
  • E901: SyntaxError or IndentationError
  • E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

@julianoes
Copy link
Contributor Author

@cclauss thanks, see: #200.

@cclauss
Copy link

cclauss commented Sep 23, 2019

Nice! PX4/PX4-Autopilot#13008 Today, there are 100 days until Python 2 end of life.

Just because it's EOL, doesn't mean it's obsolete.

Just because it's EOL means that it is going to become an attack vector for evil doers because the Python Software Foundation and Python Core Team have been very clear that they will not even fix known security vulnerabilities after 1/1/2020..

@TSC21
Copy link
Member

TSC21 commented Oct 7, 2019

@julianoes what about we keep Python2 together with Python3? I mean, instead of replacing, we duplicate the install steps for Python3

@julianoes
Copy link
Contributor Author

@TSC21 I would like to move to Python 3 as much as possible. Otherwise we'll be stuck with Python 2 forever. I suggest that add whatever Python 2 things are necessary for ROS in the ROS containers.

@TSC21
Copy link
Member

TSC21 commented Oct 7, 2019

@TSC21 I would like to move to Python 3 as much as possible. Otherwise we'll be stuck with Python 2 forever. I suggest that add whatever Python 2 things are necessary for ROS in the ROS containers.

Can we at least keep python-pip and python-setuptools? If not, can you move it to the simulation containers on this PR?

@julianoes
Copy link
Contributor Author

Can we at least keep python-pip and python-setuptools?

I wouldn't add dependencies that are not used in the same scope. Otherwise the next person will remove them again because it's not clear why they are there.

Feel free to add commits to add what ROS requires.

@TSC21
Copy link
Member

TSC21 commented Oct 7, 2019

Can we at least keep python-pip and python-setuptools?

I wouldn't add dependencies that are not used in the same scope. Otherwise the next person will remove them again because it's not clear why they are there.

Feel free to add commits to add what ROS requires.

The problem is not just ROS: http://ci.px4.io:8080/blue/organizations/jenkins/PX4%2Fcontainers/detail/PR-199/2/pipeline/#step-104-log-33

@julianoes
Copy link
Contributor Author

julianoes commented Oct 7, 2019

Alright, I'll fix it.

Yes, it's a chicken and egg problem. It also requires PX4/PX4-Autopilot#12809.

@TSC21
Copy link
Member

TSC21 commented Nov 18, 2019

@julianoes instead of mutually excluding Python 2 and 3, let's for now keep Python 2 in the container and install Python 3 in parallel. Does that work for you?

@cclauss
Copy link

cclauss commented Nov 18, 2019

With 44 days left until Python 2 end of life, why not put Python 2 in the trash and install Python 3 everywhere? Python 3 has been in existence for 13+ years.

@TSC21
Copy link
Member

TSC21 commented Nov 18, 2019

With 44 days left until Python 2 end of life, why not put Python 2 in the trash and install Python 3 everywhere? Python 3 has been in existence for 13+ years.

Because we still have a "problem" called ROS ;)

@TSC21
Copy link
Member

TSC21 commented Nov 18, 2019

@julianoes I say we give this a shot and let it go through, so we can solve the chicken and egg problem. Then we address whatever it's required. Let's unblock this - can you please set the master branch for the Jenkins stages and bring this in? Thanks!

@julianoes
Copy link
Contributor Author

@TSC21 I think we need to release this and create a tag and then reference to this, right?

@cclauss
Copy link

cclauss commented Nov 19, 2019

I thought the Python 3 thing was already solved in ROS... apt-get install python3-rospkg python3-rospkg-modules python3-catkin-pkg python3-catkin-pkg-modules

http://wiki.ros.org/rospkg
http://wiki.ros.org/catkin_pkg
https://index.ros.org/deps/page/22/name/
https://index.ros.org/deps/page/23/name/

@TSC21
Copy link
Member

TSC21 commented Nov 19, 2019

I thought the Python 3 thing was already solved in ROS... apt-get install python3-rospkg python3-rospkg-modules python3-catkin-pkg python3-catkin-pkg-modules

http://wiki.ros.org/rospkg
http://wiki.ros.org/catkin_pkg
https://index.ros.org/deps/page/22/name/
https://index.ros.org/deps/page/23/name/

Yep, but those are for ROS 2 ;)

@TSC21
Copy link
Member

TSC21 commented Nov 19, 2019

@TSC21 I think we need to release this and create a tag and then reference to this, right?

Yes. Just change the Jenkins stuff and I can do that immediately if you want.

@TSC21 TSC21 force-pushed the pr-python3 branch 2 times, most recently from be72360 to 6fae8e9 Compare November 19, 2019 10:19
@TSC21 TSC21 force-pushed the pr-python3 branch 2 times, most recently from ab279fd to dffeaa4 Compare November 19, 2019 10:59
@TSC21
Copy link
Member

TSC21 commented Nov 19, 2019

I am merging this now and tagging it so we can solve PX4/PX4-Autopilot#12809.

@TSC21 TSC21 merged commit 3a6b9b4 into master Nov 19, 2019
@TSC21 TSC21 deleted the pr-python3 branch November 19, 2019 11:09
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