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

[ENG-447] protobuf workspaces migration #725

Merged
merged 15 commits into from
Jul 21, 2022
Merged

Conversation

adisaran64
Copy link
Contributor

@adisaran64 adisaran64 commented Jun 27, 2022

Description

  • migrating protobuf code generator to protobuf workspaces
  • ticket: ENG-447
  • purpose: current proto build configuration for proto and swagger files "not reliable" (as per ticket), this PR creates a Buf Workspace in the repository and changes the iterative use of protoc -> buf build and buf generate.

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

PR review checkboxes:

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • included the correct type prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link in the PR description to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all required CI checks have passed

Code maintenance:

I have...

  • written unit and integration tests
  • added relevant godoc and code comments.
  • updated relevant documentation (docs/) or specification (x/<module>/spec/)

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code

@adisaran64 adisaran64 changed the title [ENG-447] Protobuf Workspaces Migration [ENG-447] protobuf workspaces migration Jun 27, 2022
@adisaran64 adisaran64 marked this pull request as ready for review June 27, 2022 23:07
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

looks like make proto-gen is failing. Can you look into it?

scripts/protocgen.sh Outdated Show resolved Hide resolved
buf.gen.yaml Outdated Show resolved Hide resolved
scripts/protocgen.sh Show resolved Hide resolved
scripts/protocgen.sh Show resolved Hide resolved
@adisaran64
Copy link
Contributor Author

adisaran64 commented Jun 28, 2022

I think changing the plugin from swagger to openapi is a much more involved task (not just renaming), would involve moving from protoc-gen-swagger to protoc-gen-api, swagger-combine and all swagger related tools throughout the repository would have to all be removed and updated. can change the name of the temp folder if you want, but for now will leave it matching with the plugin name.

@adisaran64
Copy link
Contributor Author

with current commit, ran make proto-gen locally with the docker container, it generates the appropriate .proto files.

@fedekunze
Copy link
Contributor

I think changing the plugin from swagger to openapi is a much more involved task (not just renaming), would involve moving from protoc-gen-swagger to protoc-gen-api, swagger-combine and all swagger related tools throughout the repository would have to all be removed and updated. can change the name of the temp folder if you want, but for now will leave it matching with the plugin name.

Sounds good. Let's create an issue for the follow up tasks

@fedekunze
Copy link
Contributor

with current commit, ran make proto-gen locally with the docker container, it generates the appropriate .proto files.

tried running it but I'm getting the following error

Failed to "build": File "proto/buf.yaml" has unknown configuration version: v1.
make: *** [proto-gen] Error 1

@adisaran64
Copy link
Contributor Author

with current commit, ran make proto-gen locally with the docker container, it generates the appropriate .proto files.

tried running it but I'm getting the following error

Failed to "build": File "proto/buf.yaml" has unknown configuration version: v1.
make: *** [proto-gen] Error 1

not sure why this is the case for you, works fine on my end, and as far as I'm aware v1 is the only valid configuration version for the .yaml files in a buf workspace. could you please let me know the specific commands that you are using so I can try to reproduce the error? @fedekunze

@fedekunze
Copy link
Contributor

@danburck @ramacarlucho can you test from your end too?

@adisaran64 adisaran64 requested a review from fedekunze July 5, 2022 18:05
@danburck
Copy link
Contributor

danburck commented Jul 8, 2022

@adisaran64 can you

  • add in the description the effects of this change,
  • link a linear ticket if there is one and
  • add a changelog?

@adisaran64 @fedekunze make proto-gen is failing for me

➜ make proto-gen
Generating Protobuf files
/usr/local/bin/docker run --rm -v /Users/danburck/code/evmos/evmos:/workspace --workdir /workspace tendermintdev/sdk-proto-gen sh ./scripts/protocgen.sh
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.
./scripts/protocgen.sh: line 54: statik: not found
make: *** [proto-gen] Error 127

@adisaran64
Copy link
Contributor Author

@danburck added those elements to the PR/changelog.
Realized that statik was already on my docker image but not installed through the script, added a line in protoc-gen.sh (run by make proto-gen to get statik (although this isn't directly related to the generation of the .pb.go files). Would you mind trying it once more?

@github-actions github-actions bot added the CI label Jul 18, 2022
@adisaran64
Copy link
Contributor Author

@danburck added build to workflow to generate code files when .proto files change

@danburck
Copy link
Contributor

Nice @adisaran64, I ran make proto-gen again. Can you have a look at the following things:

  • Looks like the docs/protocol/proto-docs.md only includes ibc core messages after the execution. Is that expected?

Screen Shot 2022-07-18 at 23 10 03

* Getting a bunch of Warnings `Warning: Duplicate generated file name "proto-docs.md". Buf will continue without error here and drop the second occurrence of this file, but please raise an issue with the maintainer of the plugin.`

Other than that the proto generation works fine 👍

@adisaran64
Copy link
Contributor Author

adisaran64 commented Jul 21, 2022

@danburck for some reason the warnings were suppressed for me, didn't notice the docs generation had issues. thanks for letting me know.

turns out the docs/protocol/proto-docs.md file was being overwritten multiple times when generating swagger documentation (the second set of buf generate calls in the loop in the scripts/protocgen.sh file). split up the buf.gen.yaml file into two files so that it's not overwritten, changed the buf strategy so that all files are collected before generating docs (so not only a single subdirectory of proto is documented).

file generated correctly now, you should be able to confirm by removing your docs/protocol/proto-docs.md file and running make proto-gen. the warnings shouldn't appear but it would be good to check, since I couldn't see them before or after.

Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

ACK Great work @adisaran64 . Tested and works fine. Note, that I pushed a commit that fixes a deprecation warning.

@danburck danburck merged commit 4b5f924 into main Jul 21, 2022
@danburck danburck deleted the adisaran64/buf-workspace branch July 21, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants