Add Bazel build files for p4c #2430
Conversation
... and small fixes.
| @@ -322,6 +322,21 @@ default is 2GB, which is not enough to build p4c. Increase the memory limit to | |||
| at least 4GB via Docker preferences or you are likely to see "internal compiler | |||
| errors" from gcc which are caused by low memory. | |||
|
|
|||
| # Bazel | |||
There was a problem hiding this comment.
This is cool.
One nice thing about the makefiles was that you could just add another project and if you respect the format of the makefiles it would build. For example, on my machine I checkout the p4c-xdp repo, and then I create a symlink in extensions to the root directory, and it is automatically picked up by cmake, including all tests. I don't really know bazel, but with this structure it doesn't look to me like this would behave in the same way.
There was a problem hiding this comment.
With Bazel this would work the opposite way, in the sense that your extension (p4c-xdp) would include p4c as a dependency. There is an example in the bazel/example folder of how to depend on p4c in a Bazel project.
EDIT: I may be misunderstanding how extensions work though.
There was a problem hiding this comment.
An extension works by adding a symlink from the extensions folder to a source directory. Then the *.def files and the Cmakefile are picked up by the build automatically. There are some conventions on the structure of the makefile. As an example you can see http://github.com/vmware/p4c-xdp. It would be nice to make sure that one can be added to the Bazel build without editing any of the files in this repository.
|
It seems that GitHub Actions are currently disabled for this repository. @mbudiu-vmw, could you take a look at enabling them? https://help.github.com/en/github/administering-a-repository/disabling-or-limiting-github-actions-for-a-repository For context, this PR adds a Github Action that tests the Bazel build for every PR/commit to master. |
|
I don't think I have permissions to enable actions. |
|
@jnfoster Nate, do you have permissions to enable them? Or can point us to someone who does? |
|
The p4lang organization was on a legacy paid plan that did not support actions. I changed that about a week ago on @antoninbas's request. However, the change won't take effect until 13 June and I don't see a way to override that. So in 2 days we should be able to do this. |
|
Interestingly they already work for p4lang/p4-constraints and p4lang/p4runtime. |
|
Weird. I must not be finding the right settings then. |
|
Weird indeed, because they should be enabled by default in the first place (though this may be explained by the legacy thing). |
|
Here's what I see for p4c...
|
|
Looks right. Could you try disabling and then reenabling? GitHub is owned by Microsoft now so it probably needs a restart ;) |
|
When I changed the settings, my whole browser froze up and turned blue... Just kidding. I disabled, waited for the green check mark to appear in the right place, and then re-enabled. Try again? |
|
Never mind, I think I know what's going on. The action is not being run because the PR is based on a branch in @EileenFeng's repository. If we want to run the action before merging, we need to move the PR to a branch on p4lang/p4c. I don't currently have the necessary privileges to do that though. EDIT: Just to be clear, the action will be run on all PRs in the future. It is just not being run now since the action itself is new and not yet merged into master. |
Changing commit message to see if this will cause GitHub action to run; ignore.
|
Alternatively, reviewers can convince themselves that this works by looking at Eileen's repository: https://github.com/EileenFeng/p4c/actions |
There was a problem hiding this comment.
Since I don't know anything about Bazel this should probably be also reviewed by someone with more experience. On the other hand, this doesn't break anything we have and adds new functionality. I think that the litmus test is whether this can be integrated with an extension. If not, it's usefulness will be limited.
| visibility = [":__subpackages__"], | ||
| ) | ||
|
|
||
| # This rule helps split some circular dependencies between subdirectories. |
There was a problem hiding this comment.
If we have circular dependencies this may be a bug, perhaps we should investigate.
Could you file an issue describing the dependency you found?
There was a problem hiding this comment.
This comment originates from the Stratum authors, so I don't know the details here.
| name = "ir_generated_files", | ||
| srcs = [ | ||
| "frontends/p4-14/ir-v1.def", | ||
| "backends/bmv2/bmv2.def", |
There was a problem hiding this comment.
Could this be factored so that the backend and the frontend are not mingled together?
Ideally the backend should be treated as an add-on that depends on the frontend.
I..e, if we add a new backend we should not need to edit the main makefile, esp. if the backend is an extension.
This is not a must, but would be nice to have.
There was a problem hiding this comment.
Are you suggestion that we invoke the ir generator twice? Is that possible? If so I'm happy to refactor things that way if you'd prefer.
| @@ -322,6 +322,21 @@ default is 2GB, which is not enough to build p4c. Increase the memory limit to | |||
| at least 4GB via Docker preferences or you are likely to see "internal compiler | |||
| errors" from gcc which are caused by low memory. | |||
|
|
|||
| # Bazel | |||
There was a problem hiding this comment.
An extension works by adding a symlink from the extensions folder to a source directory. Then the *.def files and the Cmakefile are picked up by the build automatically. There are some conventions on the structure of the makefile. As an example you can see http://github.com/vmware/p4c-xdp. It would be nice to make sure that one can be added to the Bazel build without editing any of the files in this repository.
| @@ -0,0 +1,65 @@ | |||
| """Build rule for generating C or C++ sources with Bison.""" | |||
There was a problem hiding this comment.
I don't really know how Bazel works, but it would be nice if you didn't need to copy this file to every project using bison. Is there some kind of a (trusted) repository of such rules which would allow them to be imported dynamically?
There was a problem hiding this comment.
Agreed, that would be nice. At the moment, there is no trusted rule for Bison.
bazel/p4c_deps.bzl
Outdated
| git_repository( | ||
| name = "com_github_p4lang_p4runtime", | ||
| remote = "https://github.com/p4lang/p4runtime", | ||
| commit = "776797d4bc7c2e5f1a7249f73f1c879a91688e75", |
There was a problem hiding this comment.
So we have to edit this manually every time we make a commit to p4runtime?
There was a problem hiding this comment.
Yes and no. We can refer to any p4runtime commit here, it doesn't necessarily have to be the latest one. So we do not have to update this every time we make a commit to p4runtime.
This works much like git submodules, which p4c is already using for the cmake-based build. As explained in the comment, we would ideally have this rule point at the submodule, so that cmake and Bazel would be guaranteed to use the same version of p4runtime, but due to a Bazel limitation we cannot currently do so. The good news is that if something brakes, CI will tell us.
|
Stratum comes with its own p4c backend for fixed-function pipelines. It may be a better exercise to see if they can switch over to these new targets, as opposed to doing a POC with p4c-xdp which already has a different build system (cmake based) that works fine. Then Stratum does not need to maintain its own Bazel BUILD file for p4c. Even if p4c-xdp can be updated to support both a Bazel build and a cmake build, the changes may not be neglectable. It's probably better to validate p4c Bazel support by working with a backend that already made the choice to use Bazel. The Stratum p4c backend @pudelkoM FYI |
|
That makes sense. @pudelkoM, would you wanna give this a try? I briefly tried myself but couldn't get Stratum to build out of the box |
|
Interesting effort. In general this is good and makes p4c more accessible. Here's what I found by just switching out the dependency source:
|
|
@pudelkoM thanks for the feedback. Those concerns should now be addressed. |
|
Initial compilation proceeds now, but I have found a blocker. Stratum has custom IR definitions for the FPM backend: stratum/stratum@8772c19#diff-bfd2d8e393f767320fbcc0aa008a69cb Maybe there is a way to inject them into the dependency. |
|
Indeed, this is the point of the exercise. The Makefiles pick up def files automatically without changing the base makefiles. |
|
@pudelkoM @mbudiu-vmw Are custom IR definitions the only place where p4c needs to know about extensions? This seems to be the case for Stratum at least. If that's the case I think we could figure something out to make things work. |
|
These are the hardest probably, because they generate C++ which is fed to the compiler to create the visitors, so they need to be known statically. We also do stuff like register tests to execute for each backend, but perhaps these are easier to deal with. |
|
Alright, I added support for custom IR definitions. |
| # extensions. | ||
| repo_mapping = { | ||
| # Tells p4c where to look for `:ir_extensions` target: in this project. | ||
| "@com_github_p4lang_p4c_extension" : "@example_p4_project", |
There was a problem hiding this comment.
pretty neat way to resolve that circular dependency :)
There was a problem hiding this comment.
@smolkaj BTW is repo_mapping supported for http_archive and git_repository?
It does not say so in the documentation, but this seems to imply it is supported: bazelbuild/bazel#6845. I believe the documentation is just inaccurate?
There was a problem hiding this comment.
I'll try this tomorrow.
Probs for finding this! I'm not sure yet if this is a really great idea or horribly undermining Bazel concepts :D
There was a problem hiding this comment.
It's worth noting that repo_mapping is not needed to do this. Something like new_local_repository(name = "com_github_p4lang_p4c_extension", ...) should also do the trick, though a bit less elegantly. I'll document this tomorrow since it looks like repo_mapping might indeed not be available for http_archive and git_repository.
There was a problem hiding this comment.
The mapping scheme seems to work!
Currently solving linker errors related to the move from gmp to boost::multiprecision
There was a problem hiding this comment.
I also just confirmed that repo_mapping works for http_archive and git_repository as well, so we can leave it as is.
|
The current include path for p4c headers is not optimal: #include "external/com_github_p4lang_p4c/frontends/p4/coreLibrary.h"
#include "external/com_github_p4lang_p4c/frontends/p4/methodInstance.h"Could probably fixed with a bit of |
|
@pudelkoM do you happen to know if we can make it so that a p4c prefix is used externally but not internally? |
|
@pudelkoM Actually, I may have misunderstood your question. Are you using and that works? What would you like this to look like? The following should also work: |
|
Sorry, I should have been more clear. Ideally it would look something like this: The For additional context, here is how other project's includes look like: #include "absl/synchronization/mutex.h"
#include "grpcpp/grpcpp.h"
#include "p4/v1/p4runtime.grpc.pb.h"
#include "tofino/bf_pal/bf_pal_port_intf.h" |
|
So with this, if I have a backend that uses Bazel, can I just drop it into the extension folder and then compile as usual? |
|
@pudelkoM I agree @fruffy Not quite. Things work the other way around with Bazel in the sense that your extension would have to depend on (i.e., include) p4c. Very concretely, take a look at the bazel/example folder (included in this PR) for how to set things up. |
|
Let me know if there is anything else to do here before we can merge. |
|
what is the verdict about extensions? |
|
They now work, see the |
|
this Travis build was queued 5 days ago, and prevents us from merging |
|
Sorry for the radio silence. I got wrapped up in other tasks. I think this PR is an improvement of the current state and therefore should be merged. If we (Stratum project) find other issues when switching later, we'll report them. |
|
I just added an empty commit hoping this will help Travis out of the deadlock. |
Not sure why this wasn't needed before?
* Bazel files for building p4c. Co-authored-by: Steffen Smolka <smolkaj@google.com>
Adding bazel files for p4c as discussed in p4lang/p4c#2271.