Seeding open-p4studio/docs#49
Conversation
|
Some merge seems to have gone strangely here? Also DCO check is failing. |
|
@vgurevich The merge commit needs to be removed. I can clean it up if needed. |
|
@fruffy -- yes, please! |
2dbebf1 to
cc6a351
Compare
|
@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 named 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. |
|
@jafingerhut -- I agree. Let me see if we can get #50 resolved relatively quickly. If not, I'll edit the text accordingly. |
|
@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 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>
0a683ae to
bcb8345
Compare
|
@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. |
| 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 |
There was a problem hiding this comment.
If you're using the open-source p4c compiler, the default target is (still) bmv2. You need --target tofino to compile for tofino.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
|
@jafingerhut -- I do not see unaddressed comments. You suggested I change |
Addressed long-standing (but invisible until now) comments from @jafingerhut Signed-off-by: Vladimir Gurevich <vag@p4ica.com>
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>
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>
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>
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> Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Added the
docsdirectory together with the first document, explaining how to compile P4 code.