Python 2/3 Compatibility by changlan · Pull Request #500 · NetSys/bess

changlan · 2017-06-05T21:03:38Z

This should fix #488.

Modified Python code to be 2/3 compatible
- Use Python 3 style print() with from __future__ import print_function;
- Use list comprehension instead of map and filter;
- Use items() instead of iteritems() (Yeah, it would become less efficient in Python 2);
- Use io package to read() from files, so that the resulting string is Unicode-encoded;
- Use raw_input for Python 2 and input for Python 3;
- Disable implicit relative import and use absolute import instead;
- Fix the discrepancy of tokenizer between Python 2/3 (-> is an OP token in Python 3);
- Run 2to3 over protoc generated scripts, since those scripts still use implicit relative import;
- Use raw bytes for non-Unicode literals (e.g b'\x01').
Add unit test for both Python 2/3.

codecov · 2017-06-05T22:16:24Z

Codecov Report

Merging #500 into master will increase coverage by <1%.
The diff coverage is 77%.

@@           Coverage Diff           @@
##           master    #500    +/-   ##
=======================================
+ Coverage      64%     64%   +<1%     
=======================================
  Files         198     198            
  Lines       12071   12079     +8     
=======================================
+ Hits         7766    7772     +6     
- Misses       4305    4307     +2

Impacted Files	Coverage Δ
pybess/protobuf_to_dict.py	`56% <ø> (ø)`	⬆️
pybess/module_msg.py	`100% <100%> (ø)`	⬆️
bessctl/test_samples.py	`90% <100%> (ø)`	⬆️
bessctl/test_sugar.py	`97% <100%> (ø)`	⬆️
pybess/test_protobuf_to_dict.py	`100% <100%> (ø)`	⬆️
pybess/test_bess.py	`100% <100%> (ø)`	⬆️
pybess/bess.py	`40% <47%> (ø)`	⬆️
bessctl/sugar.py	`92% <91%> (-1%)`	⬇️
core/utils/checksum_test.cc	`99% <0%> (-1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b75a71a...3b2bd70. Read the comment docs.

sangjinhan · 2017-06-05T22:19:29Z

@@ -1,24 +1,25 @@
 #!/usr/bin/env python2.7


Now all scripts should start with "#!/usr/bin/env python", so that they can run regardless of the installed Python version..?

(sorry not all scripts, but executable scripts)

sangjinhan · 2017-06-05T22:26:17Z


 install:
-  - sudo apt-get install -y python
+  - sudo apt-get install -y python python3 python3-pip


While it is not an issue on our test setup, python might be an alias of python3 on other distros. I would be better to specify python2 explicitly.

Done. python2 is not a package name, but I changed python to python2 in other places.

python2.7...?

... Done -_-||

sangjinhan · 2017-06-05T22:26:27Z

 install:
-  - sudo apt-get install -y python
+  - sudo apt-get install -y python python3 python3-pip
  - pip install grpcio scapy codecov


sangjinhan · 2017-06-05T22:27:43Z

-  - sudo apt-get install -y python
+  - sudo apt-get install -y python python3 python3-pip
  - pip install grpcio scapy codecov
+  - pip3 install grpcio coverage


(why not scapy for pip3?)

scapy for pip3 is broken :(
And the tests don't need scapy.

Oh... then perhaps scapy-python3 is the package we need to install (https://github.com/phaethon/scapy)...

Also bessctl/bessctl -- daemon start -- run testing/run_module_tests in .travis.yml now needs "python2" explicitly, as it does use scapy?

Also we would need to replace str(scapy packet) with bytes(scapy packets) in BESS scripts... The scapy3k webpage says:

N.B.! As a difference from scapy for python2, use bytes() instead of str() when converting packet to bytes. Also, most arguments expect bytes value instead of str value except the ones, which are naturally suited for human input (e.g. domain name).

sangjinhan · 2017-06-05T22:28:36Z

@@ -1,24 +1,25 @@
 #!/usr/bin/env python2.7


(sorry not all scripts, but executable scripts)

sangjinhan · 2017-06-05T22:32:10Z

        line_buf = []

        for arg in sys.argv[1:]:
+            arg = arg.decode()


Why do we convert it into a unicode..? (please add a comment)

StringIO takes unicodes. I changed it to BytesIO for clarity (and thus avoiding the conversion).

Ah, it seems that in Python 2 arguments are merely str (not unicode), while in Python 3 they are Unicode str, so we have to convert them into unicode first.

py3k str is Unicode, yes. (The type name unicode no longer exists, it's all just str.) py2k str and bytes are the same underlying type. The py2k io.StringIO does require unicode, so probably this should use io.BytesIO on py2k and io.StringIO on py3k. Or you can use six.StringIO here, if you're importing six (I forget if you are).

Yes, I guess it is easier to just make sure the input are Unicode (already did) instead of handling StringIO and BytesIO separately. I am attempting to avoid extra packages like six and future to keep the dependency minimal 😄

Sure, but you'll have to not do .decode() if it's py3k (either that, or trap AttributeError as there is no decode for str objects).

It has been fixed in 5e4a82d

Oh, clever! u' '.join(...) works in both, converting py2k string-as-bytes to unicode, leaving py3k strings alone.

sangjinhan · 2017-06-05T22:49:22Z

    FieldDescriptor.TYPE_BOOL: bool,
-    FieldDescriptor.TYPE_STRING: unicode,
+    FieldDescriptor.TYPE_STRING: str,
    FieldDescriptor.TYPE_BYTES: str,


I am not quite sure about this, but shouldn't it be bytes...?

chris3torek

I've merely scanned this for anything that stood out to me.

chris3torek · 2017-06-05T22:30:04Z

        line_buf = []

        for arg in sys.argv[1:]:
+            arg = arg.decode()


The elements of sys.argv should already be strings (not bytes) in py3k. Are you sure this is needed?

Explained in @sangjinhan's comment above.

chris3torek · 2017-06-05T22:31:41Z

@@ -111,7 +112,7 @@ def check_c_lib(lib):
 def required(header_file, lib_name, compiler):
    if not check_header(header_file, compiler):
        print >> sys.stderr, 'Error - #include <%s> failed. ' \


Seems to be missing its conversion...?

Ah, this seems to be a stale version.

chris3torek · 2017-06-05T22:33:29Z

@@ -73,9 +74,9 @@ def do_dist_clean():

 def print_usage():
    print >> sys.stderr, \


This also still has the >> sys.stderr (and is not a function call)...

chris3torek · 2017-06-05T22:53:59Z

-    FieldDescriptor.TYPE_SFIXED64: long,
+    FieldDescriptor.TYPE_SFIXED64: int,
    FieldDescriptor.TYPE_BOOL: bool,
-    FieldDescriptor.TYPE_STRING: unicode,


I don't quite understand how this is to work in both py2k and py3k but I also don't quite understand how it works now in py2k in the first place, so I'm just confessing ignorance here. 😄 (In particular the values have to already have been converted somewhere by this point.)

sangjinhan · 2017-06-07T21:52:39Z

changlan force-pushed the 2to3 branch 2 times, most recently from 5aa2591 to 82e6ddf Compare June 5, 2017 21:55

sangjinhan reviewed Jun 5, 2017

View reviewed changes

chris3torek reviewed Jun 5, 2017

View reviewed changes

changlan added the notready label Jun 5, 2017

changlan force-pushed the 2to3 branch from 455e9fd to 315467a Compare June 5, 2017 23:48

changlan removed the notready label Jun 5, 2017

azuresol added 7 commits June 5, 2017 16:52

Python 2/3 compatible code

66ed0ed

Test Python 3 compatibility

1054755

Use Python 3 print function in build scripts

e168097

Fix octal constants

5a128cf

Exception message field and raw_input()

b99655e

Allow default installed Python version

1397a81

Update scalar types for protobuf messages

00b6aad

changlan force-pushed the 2to3 branch 3 times, most recently from 5353b8d to 5e7fc4a Compare June 6, 2017 00:39

azuresol added 3 commits June 6, 2017 15:21

Explicitly use python2 and pip2

ade8bdf

Implicitly convert arguments into unicode

20b1255

Module tests with Python 3

7a5cd1f

changlan force-pushed the 2to3 branch from 5e7fc4a to 7a5cd1f Compare June 6, 2017 22:21

Run Python 3 tests only once on CI

3b2bd70

changlan force-pushed the 2to3 branch from be304a2 to 3b2bd70 Compare June 7, 2017 19:22

sangjinhan merged commit 1bd5bba into master Jun 7, 2017

changlan deleted the 2to3 branch June 7, 2017 23:45

		@@ -73,9 +74,9 @@ def do_dist_clean():

		def print_usage():
		print >> sys.stderr, \

Uh oh!

Conversation

changlan commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changlan Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris3torek Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changlan Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris3torek Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris3torek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changlan commented Jun 5, 2017 •

edited

Loading

codecov Bot commented Jun 5, 2017 •

edited

Loading

changlan Jun 6, 2017 •

edited

Loading

chris3torek Jun 6, 2017 •

edited

Loading

changlan Jun 6, 2017 •

edited

Loading

chris3torek Jun 6, 2017 •

edited

Loading