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.

Sign up for GitHub

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

Jump to bottom

Add bf pktpy source #221

Merged
merged 5 commits into from
Mar 14, 2025
Merged

Add bf pktpy source #221

merged 5 commits into from
Mar 14, 2025

Conversation

jafingerhut
Copy link
Collaborator

No description provided.

@jafingerhut
Add source code of bf_pktpy modules
dc22d46 
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut
Add Python package dependencies for bf_pktpy to setup.cfg
6d110ce 
so that when ptf is installed via `pip install ptf`, all of those
other Python packages will also be installed.

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut
Formatting changes from running black on bf_pktpy code
a353792 
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut
Make more function names usable via ptf when common in bf-pktpy and s…
4953210 
…capy

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@jafingerhut jafingerhut mentioned this pull request Mar 14, 2025
Open
@jafingerhut jafingerhut requested a review from fruffy March 14, 2025 18:41
@jafingerhut
Copy link
Collaborator Author

@fruffy All p4c tests are passing using the version of ptf as modified by this PR, plus some changes in p4c tests to use bf_pktpy instead of scapy.

You can see the p4c requirements.txt file I used here: https://github.com/p4lang/p4c/pull/5145/files#diff-4d7c51b1efe9043e44439a949dfd92e5827321b34082903477fd04876edb7552
on this p4c PR: p4lang/p4c#5145

This PR's copy of the bf_pktpy source files is the same as the latest in https://github.com/p4lang/open-p4studio, except it also has the small additions proposed in this PR: p4lang/open-p4studio#85 which enable more code in p4c tests to avoid using Scapy.

@jafingerhut
Update README for bf_pktpy being included as part of the ptf package
86f4095 
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
@fruffy fruffy requested a review from antoninbas March 14, 2025 20:08
fruffy
fruffy reviewed Mar 14, 2025
View reviewed changes
setup.cfg
install_requires =
six
getmac
scapy_helper
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this particular dependency needed for? License looks fine and it doesn't pull in scapy. Just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't looked in detail. All I know is that if you try to do import bf_pktpy without this package, you get an error that it cannot find it. It appears to define a few little functions for converting between different IP and MAC address formats, and probably a couple of other things I'm forgetting.

fruffy
fruffy reviewed Mar 14, 2025
View reviewed changes
Copy link
Contributor

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Does this also correctly export bf_pktpy as top-level module?

@jafingerhut
Copy link
Collaborator Author

Does this also correctly export bf_pktpy as top-level module?

Yes. I have done pip install ptf followed by import bf_pktpy in an interactive python3 session, and it finds the bf_pktpy module with that name. All of the existing tests linked at p4lang/p4c#5145 worked with these additions without further changes, except no longer importing bf_pktpy from the open-p4studio repo.

@fruffy
Copy link
Contributor

fruffy commented Mar 14, 2025

Sounds good. After merging this we should consider making bf_pktpy the default.

@jafingerhut
Copy link
Collaborator Author

Sounds good. After merging this we should consider making bf_pktpy the default.

We can talk about that. I'd prefer discussing it over a later separate PR, if that is OK.

@fruffy
Copy link
Contributor

fruffy commented Mar 14, 2025

We can talk about that. I'd prefer discussing it over a later separate PR, if that is OK.

No problem, let's give this PR some time so @antoninbas can have a look. Then we can merge it.

@antoninbas
Copy link
Member

We can talk about that. I'd prefer discussing it over a later separate PR, if that is OK.

No problem, let's give this PR some time so @antoninbas can have a look. Then we can merge it.

Thanks @fruffy, but unfortunately I don't really have the cycles to review PRs in this repo - especially large PRs such as this one - and I don't have all the background related to this change / bf_pktpy. Feel free to move forward without my input.

fruffy
fruffy approved these changes Mar 14, 2025
View reviewed changes
Copy link
Contributor

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Thanks @fruffy, but unfortunately I don't really have the cycles to review PRs in this repo - especially large PRs such as this one - and I don't have all the background related to this change / bf_pktpy. Feel free to move forward without my input.

Sounds good. Was mostly pinging in case you had reservations on including this code with PTF.

@jafingerhut
Copy link
Collaborator Author

@antoninbas If you are interested in reviewing this (no obligation, of course), here is some background:

In 2021-2022, Barefoot folks after the Intel acquisition were told by Intel legal/software-license folks that because Scapy is licensed GPLv2, they should do something different if they wanted to release code under the Apache 2.0 license that used ptf, because I believe at that time ptf always imported Scapy, and importing GPLv2 code from Apache-2.0 code, or vice versa, is on very legally questionable ground.

After lots of internal discussion, the decision was made to create a new module named bf_pktpy that was a clean room reimplementation of a subset of Scapy's features, using a similar Python API, for at least a large subset of features needed for Tofino ptf tests.

Before Jan 15, 2025 when Intel released the code in https://github.com/p4lang/open-p4studio, bf_pktpy was only available to people who got the full proprietary Intel P4Studio SDE (as far as I know). On that date, bf_pktpy code was released under Apache-2.0 license as part of open-p4studio, here: https://github.com/p4lang/open-p4studio/tree/main/pkgsrc/ptf-modules/bf-pktpy

That repo is about 1 GByte of code to clone it. The only known use case for bf_pktpy is in association with ptf. Thus Fabian's suggestion that we copy bf_pktpy code into the ptf repo, to make it quicker and more convenient to use when one uses ptf, since with these changes the bf_pktpy module(s) will be installed when you do pip install ptf.

The code is pretty much a straight copy from the open-p4studio repo, plus changes to make the black formatter happy, plus a dozen-line addition I made to make more names visible in the bf_pktpy module that is imported by ptf.packet, when you set up bf_pktpy as your packet manipulation module.

@jafingerhut
Copy link
Collaborator Author

Being brave and hitting the merge button on this one.

@jafingerhut jafingerhut merged commit e29ef7e into p4lang:main Mar 14, 2025
9 checks passed
@jafingerhut jafingerhut mentioned this pull request Mar 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy approved these changes

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jafingerhut @fruffy @antoninbas