Skip to content

Seeding open-p4studio/docs#49

Merged
jafingerhut merged 7 commits into
mainfrom
vgurevich/docs
Jun 23, 2025
Merged

Seeding open-p4studio/docs#49
jafingerhut merged 7 commits into
mainfrom
vgurevich/docs

Conversation

@vgurevich

Copy link
Copy Markdown
Contributor

Added the docs directory together with the first document, explaining how to compile P4 code.

Comment thread docs/P4-Compilation.md
Comment thread docs/P4-Compilation.md Outdated
@jafingerhut

Copy link
Copy Markdown
Contributor

Some merge seems to have gone strangely here? Also DCO check is failing.

@fruffy

fruffy commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

@vgurevich The merge commit needs to be removed. I can clean it up if needed.

@vgurevich

Copy link
Copy Markdown
Contributor Author

@fruffy -- yes, please!

@vgurevich

Copy link
Copy Markdown
Contributor Author

@jafingerhut -- can you, please review?

@jafingerhut

jafingerhut commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

@jafingerhut -- can you, please review?

So it is still written in a way that reflects the way you hope that things will be later, not the way things are now, yes?

So anyone reading it, and believing it, will get confused.

Why not call what is namedp4c right now p4c in this article? If it changes to bf-p4c later, then this article can be updated, either in the same commit or soon afterwards.

Alternately, when you think the rest of the repository has been modified such that this article's contents are accurate, I am happy to review it in detail based upon trying out all the steps on the current build at that future time.

@vgurevich

Copy link
Copy Markdown
Contributor Author

@jafingerhut -- I agree. Let me see if we can get #50 resolved relatively quickly. If not, I'll edit the text accordingly.

@vgurevich

Copy link
Copy Markdown
Contributor Author

@jafingerhut -- I finally got some time to update the text. Please, take a look when you can.

@jafingerhut

Copy link
Copy Markdown
Contributor

@jafingerhut -- I finally got some time to update the text. Please, take a look when you can.

I am building a fresh build of open-p4studio on a personal VM to try out the new instructions and verify their accuracy again, is why you see me making a flurry of other issues and PRs discovered while doing that. I had not tried install.sh in quite a while.

When that build succeeds and I try out the steps in this doc again, I will add my review comments here.

Created the docs directory for miscellaneous documentation

Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Added the information about the additional `-DP4C=` flag.
Also, replaced typographic quotation marks with ASCII quotes for the easier copy-paste

Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Addressed the feedback regarding `bfrt.json` filename.
Fixed a couple of typos.
Added a couple of clarifications

Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Edited the file to remove references to `bf-p4c` and replace them with `p4c` (thereby adding one more mandatory parameter to `cmake` invocation). 

Removed references to P4ica tools until the compiler naming issue is resolved.

Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
@vgurevich

Copy link
Copy Markdown
Contributor Author

@jafingerhut -- is there anything I need to do to to make it possible to merge? I wanted to write another doc, but I'd like to get this in first.

Comment thread docs/P4-Compilation.md
On the surface, the P4 compiler for Tofino (`p4c`) works like any other compiler. In other words, it is totally ok to compile your program using the following command line:

```
p4c my_program.p4

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you're using the open-source p4c compiler, the default target is (still) bmv2. You need --target tofino to compile for tofino.

@vgurevich vgurevich Jun 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Chris! There is still a confusion going on in this repo with regards to the compiler name.

For whatever reason someone decided to name it p4c (so that in $SDE_INSTALL/bin there is a file named p4c and not bf-p4c, even though in reality it is the Python wrapper developed at Barefoot/Intel with the actual compiler being p4c-barefoot, of course.

As a result doing what I said (p4c my_program.p4) does work.

I am not sure who decided to change the name and who should be persuaded to change it back, but your comment clearly indicates the need.

I filed a case for that (#50 ) quite a while ago. Maybe you can weigh in.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

p4c is the generic driver program that figures out which backend to run, based on the --target option. You can configure the default target to use if there is no --target option to anything, but different builds do it differently.

IMO we should encourage people to ALWAYS use the --target option -- that way things "just work" regardless of how it was installed. Specifying it when it is not needed (because that target was set up as the default) is never harmful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ChrisDodd,

Thank you. I see that the main driver itself (bf-p4c in the SDE and p4c in the open-p4studio) is the same. Also, in both cases the directory $SDE_INSTALL/share/p4c/p4c_src` contains only Tofino-specific files and does not contain the plugins for the other backends.

That explains why the command line above works and confirms my assertion that p4c included in open-p4studio is not exactly the "stock" one. I think that it is po

As for your second point, I do not think that people should be supplying arguments for which there are reasonable defaults. Yes, the option os there, but for the simple things people should be typing very simple commands.

We do not routinely specify -m parameter(s) when compiling simple using GCC, do we? In embedded world people often use things like p4c-gcc, etc. so allowing people to do p4c myprog.p4 (when they are clearly working in the Tofino environment) or bf-p4c myprog.p4 isn't such a bad thing, is it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Chris, not trying to ignore your comments, but FYI I am approving this PR as it is right now, now that we figured out why some of my typo-level comments were not visible to others before, and Vladimir has fixed them with a commit a few minutes ago as I write this.

My main concern is that the documentation is accurate for people installing the open-p4studio code, and it appears to me these instructions are accurate for that. I have not personally tried installing only open source p4c and compiling for Tofino, and you may be correct that using --target tofino is required and/or a good idea in that situation, but that is definitely not required if you install open-p4studio.

@jafingerhut

Copy link
Copy Markdown
Contributor

@jafingerhut -- is there anything I need to do to to make it possible to merge? I wanted to write another doc, but I'd like to get this in first.

Do you see the unaddressed review comments? I wrote them some months back, and then re-reviewed those comments, resolved a couple of them about a week or two ago when you mentioned this issue, but as far as I can tell, there are still several comments that are relevant, asking questions or recommending changes.

@vgurevich

Copy link
Copy Markdown
Contributor Author

@jafingerhut -- I do not see unaddressed comments. You suggested I change bf-p4c to p4c and I did. The last comment from you (months ago) was that you'll try it on your VM and let me know. Am I missing something?

Comment thread docs/P4-Compilation.md
Comment thread docs/P4-Compilation.md Outdated
Comment thread docs/P4-Compilation.md Outdated
Comment thread docs/P4-Compilation.md Outdated
Comment thread docs/P4-Compilation.md Outdated
Comment thread docs/P4-Compilation.md
Addressed long-standing (but invisible until now) comments from @jafingerhut 

Signed-off-by: Vladimir Gurevich <vag@p4ica.com>

@jafingerhut jafingerhut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jafingerhut jafingerhut merged commit ced579f into main Jun 23, 2025
2 checks passed
@jafingerhut jafingerhut deleted the vgurevich/docs branch June 23, 2025 22:21
n1tr0-5urf3r pushed a commit to n1tr0-5urf3r/open-p4studio that referenced this pull request Jun 25, 2025
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: Fabian Ihle <fabian.ihle@uni-tuebingen.de>
n1tr0-5urf3r pushed a commit to n1tr0-5urf3r/open-p4studio that referenced this pull request Jun 26, 2025
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: Fabian Ihle <fabian.ihle@uni-tuebingen.de>
n1tr0-5urf3r pushed a commit to n1tr0-5urf3r/open-p4studio that referenced this pull request Jun 27, 2025
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: Fabian Ihle <fabian.ihle@uni-tuebingen.de>
rcgoodfellow pushed a commit to oxidecomputer/tofino-sde that referenced this pull request Feb 21, 2026
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
jafingerhut pushed a commit to jafingerhut/open-p4studio that referenced this pull request Feb 26, 2026
Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
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.

4 participants