p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
140 stars 86 forks source link

Add generated rust code #483

Open duskmoon314 opened 1 month ago

duskmoon314 commented 1 month ago

The rust code is generated using the neoeinstein/protoc-gen-prost, which leverages the prost crate for protobuf and tonic crate for client/server.

duskmoon314 commented 1 month ago

I have also created a client wrapper based on this generated code. A basic example of inserting table entries and receiving digests with the wrapper is also available.

See duskmoon314/p4runtime-client-rs


Please let me know if there is anything I need to do to push this further.

chrispsommers commented 1 month ago

Hi @duskmoon314 Sorry for the delay. I've run your workflow and it passes. Thank you for your contribution. Given that rust is a new client binding for P4RT perhaps it would be best if you joined the next WG meeting (June 14th 2024) and presented this to the group so we can ask questions. Also, I'm not sure who else in the WG has enough Rust experience to properly review the submission. If anyone wants to volunteer as a reviewer it'd be appreciated!

duskmoon314 commented 1 month ago

Thanks for your feedback and invitation. Unfortunately, the meeting time is midnight for me (UTC +8). I'm afraid I cannot attend the meeting.

I think I can prepare some documents on how the scripts work and what code is generated. Thus it would be convenient for the WG to know whether this is appropriate.

I will also announce this PR in the Rust community later and hope to find someone familiar with gRPC who is interested in reviewing it.

duskmoon314 commented 4 weeks ago

I have created a gist of my notes on this PR: https://gist.github.com/duskmoon314/59ca06dd5cac6ed00a45a112d1ef5ba6

I may forget to include some details. Please point out if I need to add them to the notes.

chrispsommers commented 3 weeks ago

Hi @duskmoon314 thanks for the gist above, it is very detailed. We discussed your PR in today's WG meeting. We came to some conclusions:

  1. A Rust client seems like a welcome addition to P4RT
  2. We'll try to find another Rust practitioner (one who understands P4Runtime as well) to review the PR.
  3. There is a question as to whether it is necessary to check in the generated Rust code at all. Scripts etc. used to generate the code, and a CI action to verify the code generates w/o errors, are clearly valuable. For reasons we couldn't recall in the meeting today, we check in the golang client code as a special case. It does create a bit more maintenance work. (Note, we don't check in generated Python code, for example.) I'd ask @antoninbas or another "P4RT historian" to refresh our memory as to why we made this exception. Does the same reasoning apply to generated Rust code?
antoninbas commented 3 weeks ago

The generated Golang code should be checked in, as this is the common practice and is based on how Golang dependencies are imported / consumed. When a downstream project wants to build a Golang application that includes a P4Runtime client, they only need to import github.com/p4lang/p4runtime as a dependency, and they get access to the generated client code. Golang does not rely on publishing binary artifacts, the code is imported directly from source version control remotes and built as part of the project which depends on it. For example, Kubernetes checks-in all the generated client code (e.g., https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/node/v1/generated.pb.go). There is no central registry for Golang where modules are published. And asking all projects to generate their own client code is not typically done.

Python works differently, but it should be noted that we still check-in the code: https://github.com/p4lang/p4runtime/tree/main/py/p4. IMO, it is not strictly required however, and the code is not consumed directly from version control. Instead, we could generate the code on the fly before uploading to PyPI.

For Rust, it would seem that checking-in the generated code (like we do for Golang) would actually be the best approach. While we could just generate a crate on the fly and publish it to crates.io, it seems quite convenient to consume them directly from Github (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories). IMO, that would be less of a burden that dealing with crates.io (the 2 are not mutually exclusive).

duskmoon314 commented 3 weeks ago

Thanks for your feedback!

There is a question as to whether it is necessary to check in the generated Rust code at all.

I mimicked the go approach in creating this PR. I think checking in the generated code has two pros:

  1. People can view the generated code. (Though this may not be very useful.)
  2. Rust can use a crate (library, package) directly from GitHub (or other remote repo). Thus, users can test an unreleased main branch easily.

It does create a bit more maintenance work.

This, indeed, is the con of checking in the code. I hope to hear more details about the maintenance work of the generated Go code and see if we can make similar work easier in Rust.

chrispsommers commented 3 weeks ago

@antoninbas Thanks so much for the very detailed reply (and correcting my misconception about Python generated code); it was most helpful!

@duskmoon314 Based on Antonin's response and your own follow-up, I'm now firmly in favor of you checking in the generated Rust code for all the reasons cited. As far as "maintenance," it's not much, see this from the README. It is not uncommon for PR submitters to forget to do this step and the CI build fails. Other than that, the maintenance is minor.

When updating the Protobuf files in a pull request, you will also need to update the generated Go and Python files, which are hosted in this repository under go/ and py/. This can be done easily by running ./codegen/update.sh, provided docker is installed and your user is part of the "docker" group (which means that the docker command can be executed without sudo).

As part of your PR, can you update the README to include Rust generated code in the text, and also update the contents of codegen/ to regenerate the Rust code as well?

The only other matter is asking someone with some Rust knowledge to review this so we have some technical assurance. I've no doubts, but I'm not qualified.

duskmoon314 commented 3 weeks ago

and also update the contents of codegen/ to regenerate the Rust code as well?

I'm sorry, but I don't quite understand what "regenerate" means here. Should I regenerate the Rust code on my machine, or do some scripts in codegen need to be modified?

chrispsommers commented 2 weeks ago

I went back and realized you'd already modified the scripts to generate the code, disregard my comment. Sorry for the confusion! It looks good to me, we just need someone to review the Rust aspects. Thanks!