Include header in text proto files

[smolkaj]

p4c generates protobuf files in text format. E.g,. when we call

    p4test --arch v1model \
        --p4runtime-files p4info.txt \
        --p4runtime-entries-files entries.txt \
        table-entries-ternary-bmv2.p4

then p4info.txt and entries.txt are both protobuf files in text format.
It is good practice to include a header in text proto files.

Proposal

P4Info

# proto-file: p4info.proto
# proto-message: P4Info
...

Entries File

# proto-file: p4runtime.proto
# proto-message: WriteRequest
....

Context

This came up in p4lang/p4runtime#432 (comment), where it wasn't clear what the schema of the table entries file was. A header would have helped to quickly resolve this. @jafingerhut for visibility.

[jafingerhut]

Thanks for the issue, Steffen. I will point out that there are many consumers of the current P4Info text file format today (whether that was a good design decision to do so, I leave to a separate discussion). Thus changes to it will likely lead to updates required in those many consumers.

[smolkaj]

Do we expect that some of these customer parse the P4Info files manually, instead of using the protobuf library?
As long as they use a standard-compliant protobuf text format parser, like the one provided by the official library, there would not be an issue.

EDIT: In particular, anything after a # is just a comment in text proto format.

[jafingerhut]

Perhaps. You can check what the behavioral-model simple_switch_grpc, does, for at least one public example.

[smolkaj]

This BMv2 test helper function does use the official parser: https://github.com/p4lang/behavioral-model/blob/6ee70b5eff7f510b32c074aaa4f00358f594fecb/targets/simple_switch_grpc/tests/utils.cpp#L95-L103.

Perhaps. You can check what the behavioral-model simple_switch_grpc, does, for at least one public example.

If the P4Info file is configured via grpc, using the SetForwardingPipelineConfig service, then no manual parsing would be needed at all since grpc scaffolding will take care of it already. So not sure where to look?

[smolkaj]

@fruffy Does P4TestGen do any protobuf text file parsing? Does it support comments?

[fruffy]

@fruffy Does P4TestGen do any protobuf test file parsing? Does it support comments?

No, we do not do any parsing, we just generate .proto files using a template.

[jafingerhut]

If the P4Info file is configured via grpc, using the SetForwardingPipelineConfig service, then no manual parsing would be needed at all since grpc scaffolding will take care of it already. So not sure where to look?

If you wanted to make a test change to a textproto format P4Info file to add the wrapper you are suggesting, and then load that into the existing simple_switch_grpc, and everything works, then that is one less consumer to be worried about.

In case you cannot tell, I am a big fan of actually trying it out to see if it works, rather than theorizing what we think ought to work :-)

[smolkaj]

Makes sense :) Just trying to understand how to load the P4Info. I’m used to doing it via P4Runtime, but I suspect there might also be a CLI flag? And perhaps also for table entries?

…

On Wed, Jul 19, 2023 at 12:31 Andy Fingerhut ***@***.***> wrote: If the P4Info file is configured via grpc, using the SetForwardingPipelineConfig service, then no manual parsing would be needed at all since grpc scaffolding will take care of it already. So not sure where to look? If you wanted to make a test change to a textproto format P4Info file to add the wrapper you are suggesting, and then load that into the existing simple_switch_grpc, and everything works, then that is one less consumer to be worried about. In case you cannot tell, I am a big fan of actually trying it out to see if it works, rather than theorizing what we think ought to work :-) — Reply to this email directly, view it on GitHub <#4070 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSVS4VH36FOM4AA3IB3DJ3XRAYZ5ANCNFSM6AAAAAA2OYCEFQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[fruffy]

At least for PTF tests, which use simple_switch_grpc the p4info file is loaded using SetForwardingPipelineConfigRequest.

I am not sure about other targets. It is unclear who is using P4Info at this point.

[jafingerhut]

Antonin Bas said in our last P4 API work group meeting that simple_switch_grpc does not use the entries file, so my suggestion of loading a hand-edited entries file into simple_switch_grpc to see if it is incompatible is not useful. Sorry about that.

He said that const entries are included in the BMv2 JSON file output by p4c-bm2-ss, regardless of whether you create a separate entries file, or not.