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 python3 syntax #1

Merged
merged 13 commits into from Mar 18, 2022
Merged

Fix python3 syntax #1

merged 13 commits into from Mar 18, 2022

Conversation

aweimeow
Copy link
Contributor

@aweimeow aweimeow commented Sep 18, 2019

Yi: Originally this PR includes a change to the base container images. We will work on this part when we are updating the base image to Debian10.
For this PR we just fix some code syntax for Python3.

Original PR
Due to some weird reason, the container built by 2nd step will have the ANSI as default encoding, so I need to install locales and generate UTF-8 encoding for mininet script's emoji (☠️ & ⚡️). If we didn't change the encoding to UTF-8, it may cause python encoding exception.

Also includes the following changes:

  • bump bazel version to 4.2.1
  • bump default python version to 3.8.12
  • completely remove python2
  • Removee deprectaed dependency

@aweimeow aweimeow changed the title Update image installed package and code to python3 Update image installed packages and code to python3 Sep 18, 2019
@codecov
Copy link

codecov bot commented Sep 18, 2019

Codecov Report

Merging #1 (787412f) into main (29f3915) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage   79.20%   79.20%           
=======================================
  Files         339      339           
  Lines       30928    30928           
=======================================
  Hits        24496    24496           
  Misses       6432     6432           

@ccascone
Copy link
Member

Hi @aweimeow, under which circumstances do you see python encoding exceptions? stratum.py has already the UTF-8 encoding special comment at the beginning of the file: https://github.com/stratum/stratum/blob/master/tools/mininet/stratum.py#L1

Bumping to python 3 might be abreaking change for anyone using this Docker image to run custom Mininet scripts. I'd rather find a fix that does not involve upgrading the Python version, or simply remove the emojis from the script...

@Yi-Tseng
Copy link
Contributor

Yi-Tseng commented Sep 24, 2019

I think we can ask developers to move to py2 to py3, and officially drop python2 from this image on 2020/01/01.
The code change looks fine, it should work on both py2 and py3.

@aweimeow
Copy link
Contributor Author

Hello @ccascone, although the stratum.py has the encoding header, if the terminal encoding didn't compatible with emoji character, it'll cause exception finally, here is a proof of concept.

aweimeow@MacBook-Pro:[~]$ cat test.py
# -*- coding: utf-8 -*-

print("☠️")
aweimeow@MacBook-Pro:[~]$ echo "$LANG $LC_CTYPE"
zh_TW.UTF-8 zh_TW.UTF-8
aweimeow@MacBook-Pro:[~]$ python test.py
☠️
aweimeow@MacBook-Pro:[~]$ LANG=ANSI LC_CTYPE=ANSI python test.py
Traceback (most recent call last):
  File "test.py", line 3, in <module>
    print("\u2620\ufe0f")
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-1: ordinal not in range(128)

I have no idea about which package or what command makes the new container's environment doesn't have UTF-8's encoding, so that is the reason for installing locales package to generate the corresponding language locale file.


About the upgrading to Python 3, I think it's necessary. There is a lot of famous Python 3rd library announced they won't support Python 2 after Python 2's EoL, despite stratum.py don't have many dependencies, but if the 3rd libraries have something wrong, we also can't get support on the failure.

From now to the Python 2 EoL, there have 3 months to go, if the upgrade causes the problem, the team still has time to fallback / fix the issue :)

tools/mininet/Dockerfile Outdated Show resolved Hide resolved
@pudelkoM
Copy link
Member

Closed due to inactivity. Feel free to reopen if you want to continue to work on this.

@Yi-Tseng
Copy link
Contributor

Let's reopen this pull request since we think it is better to move to python3

@Yi-Tseng Yi-Tseng reopened this Sep 24, 2020
@Yi-Tseng Yi-Tseng linked an issue Sep 24, 2020 that may be closed by this pull request
1 task
@pudelkoM pudelkoM force-pushed the master branch 2 times, most recently from f3bdc8b to 9672fbd Compare November 13, 2020 01:35
@bocon13 bocon13 added this to the 2020-12 Release milestone Dec 7, 2020
@Yi-Tseng Yi-Tseng requested a review from bocon13 December 7, 2020 23:57
.circleci/config.yml Outdated Show resolved Hide resolved
bocon13
bocon13 previously approved these changes Dec 17, 2020
Base automatically changed from master to main February 18, 2021 21:10
@Yi-Tseng
Copy link
Contributor

TODO:
delete the update_config.py since we already have pipeline builder

bocon13
bocon13 previously approved these changes Jul 27, 2021
Copy link
Member

@bocon13 bocon13 left a comment

Choose a reason for hiding this comment

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

lgtm

Yi Tseng added 4 commits November 11, 2021 14:52
 - bump bazel version to 4.0.0
 - bump default python version to 3.8.12
 - completely remove python2
 - Bump rules_python to version 0.5.0
 - Removee deprectaed dependency
 - Also fixes stratum#209
@Yi-Tseng Yi-Tseng changed the title Update image installed packages and code to python3 Fix python3 syntax Mar 17, 2022
@bocon13 bocon13 merged commit 5463313 into stratum:main Mar 18, 2022
@aweimeow
Copy link
Contributor Author

🎉🎉🎉

@bocon13
Copy link
Member

bocon13 commented Mar 18, 2022

Finally PR #1 is merged!! Thanks @aweimeow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infra Things related to CI/CD, build, and tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to Python3
5 participants