-
Notifications
You must be signed in to change notification settings - Fork 108
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
Some changes to enable proto/ptf test to pass on system using Python3 #543
base: main
Are you sure you want to change the base?
Conversation
and avoiding the use of Python2.
There may be better Python3-ish ways to implement some of these changes, so bug reports and suggestions for improvements are welcome. I was able to install open source P4 dev tools on an Ubuntu 20.04 system, with NO Python2 ever installed on the system, only Python3, and on such a system changes like the ones in this PR were needed in order for the tests in proto/ptf to pass. |
There is also this p4lang repo that is using Python 2.7. This repo could use a move to Python 3 as well - thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jafingerhut thanks for working on this. Based on the comments above, I assume you already validated the changes by following the instructions in https://github.com/p4lang/PI/blob/main/proto/ptf/README.md to run the tests?
proto/ptf/base_test.py
Outdated
if length == 0 and n == 0: | ||
length = 1 | ||
while True: | ||
try: | ||
s = n.to_bytes(length, byteorder='big') | ||
return s | ||
except OverflowError: | ||
length = length + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As indicated in the comment, this code was equivalent to the int.to_bytes
Python3 function, so I think we should just use that (and forget about Python2 support)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2nd commit has another variation of stringify() for your consideration. It doesn't behave exactly as int.to_bytes does alone, because I believe the Python2 version would take a parameter n that did not fit into the given number of bytes, and return a longer string with no exception. int.to_bytes would throw an exception in that scenario.
proto/ptf/ptf_runner.py
Outdated
@@ -117,7 +117,7 @@ def run_test(config_path, p4info_path, grpc_addr, device_id, | |||
os.environ['PYTHONPATH'] += ":" + pypath | |||
else: | |||
os.environ['PYTHONPATH'] = pypath | |||
for iface_idx, iface_name in port_map.items(): | |||
for iface_idx, iface_name in list(port_map.items()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for that change, the 2to3 tool is being conservative here but this transformation is not needed for iteration cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed that part of the changes.
The ptf repo already supportds Python3, otherwise Andy would not have been able to run the tests on a Python3 system |
I have run those tests, and all of them pass, except for the test l3_host_fwd.BadMatchKeyTest, which begins to fail because with the latest PI code, it accepts shorter byte strings than the full width. In addition to failing that reason, the code of the _AssertP4RuntimeErrorContext tries to read a field called |
Antonin, I don't know who uses these Python source files modified by this PR -- if they have always been used by copying them and pasting them into different projects with custom edits in each one, then merging these changes will not affect them, i.e. they can choose when to use Python3 instead of Python2 on their own schedule. If someone is using these files "directly", changes like these might make it difficult for them to use the latest version of this PI repository, if they still want Python2 for a while longer. |
and avoiding the use of Python2.