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

Continuing the implementation of initial entries support in p4c #4080

Merged
merged 8 commits into from
Aug 16, 2023

Conversation

jafingerhut
Copy link
Contributor

@jafingerhut jafingerhut commented Jul 23, 2023

These changes update the contents of P4Info and table entries files as generated by p4c, to match what has been agreed upon by the P4 API work group in this pull request
p4lang/p4runtime#432

This PR is intended to make a significant step towards resolving issue #4016 but will not do so completely, as this PR does not address the parts of that issue suggesting the creation of STF or PTF tests that exercise this new feature.

These changes update the contents of P4Info and table entries files as
generated by p4c, to match what has been agreed upon by the P4 API
work group in this pull request
p4lang/p4runtime#432
@jafingerhut
Copy link
Contributor Author

Note: This PR will probably fail CI tests with compile-time errors until after this PR has been merged in: p4lang/p4runtime#432

and then after that, it will likely continue to fail until the control-plane/p4runtime sub-module of p4c is updated to a commit SHA that includes the changes in the PR linked above.

@fruffy
Copy link
Collaborator

fruffy commented Jul 25, 2023

I believe #4056 may be needed to be able to update P4Runtime since the protos use experimental features.
This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set

@jafingerhut jafingerhut marked this pull request as draft July 28, 2023 16:57
@jafingerhut
Copy link
Contributor Author

@rst0git Several hours ago there was a commit to the repo https://github.com/p4lang/p4runtime, which this PR depends upon. It appears that some of these tests use apt to install p4lang-pi and some other precompiled packages, which may need updating in order for these tests to pass. Do you know if that is true?

rst0git added a commit to rst0git/PI that referenced this pull request Aug 12, 2023
This update is required to update the packages used by the CI tests for p4c.
p4lang/p4c#4080

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
rst0git added a commit to rst0git/PI that referenced this pull request Aug 14, 2023
This update is required to update the packages used by the CI tests for p4c.
p4lang/p4c#4080

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
antoninbas pushed a commit to p4lang/PI that referenced this pull request Aug 14, 2023
This update is required to update the packages used by the CI tests for p4c.
p4lang/p4c#4080

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@jafingerhut
Copy link
Contributor Author

@rst0git Thank you for creating p4lang/PI#601 and getting it through review and merging.

For my own knowledge, how long after that is merged should I wait before triggering CI builds on this PR again, such that it should "see" the new versions?

@rst0git
Copy link
Member

rst0git commented Aug 15, 2023

@jafingerhut Would you be able to update the P4Runtime commit in CMakeLists.txt?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b7bc5620a..c6117c182 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -383,7 +383,7 @@ set(FETCHCONTENT_QUIET OFF)
 FetchContent_Declare(
   p4runtime
   GIT_REPOSITORY https://github.com/p4lang/p4runtime.git
-  GIT_TAG        d76a3640a223f47a43dc34e5565b72e43796ba57
+  GIT_TAG        1e771c4e05c4e7e250df00212b3ca02ee3202d71
   GIT_PROGRESS TRUE
 )
 FetchContent_MakeAvailable(p4runtime)

@jafingerhut
Copy link
Contributor Author

@rst0git There isn't any big urgency, but if you happen to have some time and interest, do you have any knowledge of why so many of the tests would be failing with the changes in this PR? I have rebased it to latest main version as of 2023-Aug-15, plus my few changes.

@jafingerhut
Copy link
Contributor Author

@rst0git Sorry for the noise, but I believe I may have discovered the reasons for failing tests myself -- good old fashioned "run all the tests locally on your own system and debug them" approach worked. Sigh.

bazel/p4c_deps.bzl Outdated Show resolved Hide resolved
Co-authored-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@jafingerhut
Copy link
Contributor Author

@smolkaj @chrispsommers This is intended to update open source p4c to generate P4Info files with the recent changes made by this PR on the P4Runtime API spec to support initial entries on tables (the ones declared with entries, but without the const before them): p4lang/p4runtime#432

The changes are:

  • for every Table message in the P4Info file generated, include the field value has_initial_entries: true if there is an entries or const entries table property with 1 or more entries in the list. has_initial_entries is false if a table explicitly declares an empty list of entries.

  • for every table entry in the "entries" output file that is optionally generated by p4c, as now documented in the new appendix of the P4Runtime API spec, the field is_const is true if either: (a) the entry is part of a const entries property, or (b) the entry is part of an entries property without const before it, but the individual entry has a const qualifier before it.

@jafingerhut jafingerhut marked this pull request as ready for review August 16, 2023 15:24
@smolkaj
Copy link
Member

smolkaj commented Aug 16, 2023

Nice!! This LGTM.

@jafingerhut
Copy link
Contributor Author

Got one approval from a P4 API work group co-chair in a comment, and another approval from p4c developer, so merging.

@jafingerhut jafingerhut merged commit bfef774 into p4lang:main Aug 16, 2023
16 checks passed
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