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

Refactor GoVPP usage and generation #22

Merged
merged 13 commits into from
Mar 3, 2023
Merged

Conversation

ondrej-fabry
Copy link
Contributor

@ondrej-fabry ondrej-fabry commented Jan 17, 2023

This PR contains:

  • remove redundant helper functions
  • remove per-request mutex locking
  • switch from using channels for requests to using generated RPC client
  • make all errors consistent created with std Go package
  • update GoVPP to WIP of generator enhancements

CC @sknat

Copy link
Contributor

@sknat sknat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall lgtm,
We should probably decide whether to keep the generate.log here or move it to GoVPP / Calico/VPP. Same thing for the GenerateFile() interface

}

func createGenerateLog(apiDir string, fname string) {
/*func createGenerateLog(apiDir string, fname string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's debatable whether we want to keep this or not. I'd arguee it's always good to have a trail of what generated the bindings. The file headers can serve for this as well, the good thing about having a dedicated file though is that it can be parsed/copied to a container where the file headers are more for human consumption.

@@ -57,7 +51,9 @@ func init() {

}

func GenerateFile(gen *binapigen.Generator) {
func GenerateFile(gen *binapigen.Generator, file *binapigen.File) *binapigen.GenFile {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a Govpp comment, but anyway, the issue with this is we are going to be called once for every file even though the generation we are doing does not depend on the files being passed.
That's why I originally went with changing the signature for GenerateFile in https://github.com/FDio/govpp/pull/68/files#diff-4d2bacb13fc145b9d2f0e4f6a3771998ef6f3ff8d78dde2682c577febfa0f16eR30

@ondrej-fabry ondrej-fabry requested a review from sknat March 3, 2023 09:05
@sknat
Copy link
Contributor

sknat commented Mar 3, 2023

lgtm, thanks @ondrej-fabry !

@sknat sknat merged commit a2c21fe into calico-vpp:master Mar 3, 2023
@ondrej-fabry ondrej-fabry deleted the refactor branch March 6, 2023 11:55
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.

2 participants