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

Some changes to enable proto/ptf test to pass on system using Python3 #543

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jafingerhut
Copy link
Contributor

and avoiding the use of Python2.

@jafingerhut
Copy link
Contributor Author

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.

@hesingh
Copy link

hesingh commented Apr 3, 2021

There is also this p4lang repo that is using Python 2.7. This repo could use a move to Python 3 as well - thanks.

https://github.com/p4lang/ptf

Copy link
Member

@antoninbas antoninbas left a 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?

Comment on lines 66 to 73
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
Copy link
Member

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)

Copy link
Contributor Author

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.

@@ -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()):
Copy link
Member

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

Copy link
Contributor Author

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.

@antoninbas
Copy link
Member

There is also this p4lang repo that is using Python 2.7. This repo could use a move to Python 3 as well - thanks.

https://github.com/p4lang/ptf

The ptf repo already supportds Python3, otherwise Andy would not have been able to run the tests on a Python3 system

@jafingerhut
Copy link
Contributor Author

@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?

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 expected from self, and there is none. I don't know what that should be changed to, if anything -- it looks like a code path that may not have been exercised for a while?

@jafingerhut
Copy link
Contributor Author

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.

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.

3 participants