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

add latest buf generation #1614

Closed
wants to merge 5 commits into from
Closed

add latest buf generation #1614

wants to merge 5 commits into from

Conversation

nghuyenthevinh2000
Copy link
Member

Closes: #XXX

Protobuf generation with buf has gone a long way.

buf latest version: 1.4.0 has deleted "buf protoc".

the last version of buf that still supports "buf protoc" is: https://github.com/bufbuild/buf/releases/tag/v1.0.0-rc12

this creates frustration for engineers using latest version of buf since they will have to first figure out why "buf protoc" is gone when running ancient protocgen.sh

this pr intends to upgrade osmosis proto generation to latest with benefits:

  1. no more complex proto generation script
  2. using more clear buf.yaml structure
  3. up - to - date buf binary with "buf protoc" removed
  4. leverage "Buf Schema Registry" to include cosmos-sdk, cosmos-proto, googleapis proto files remotely in generating osmosis proto

Add a description of the overall background and high level changes that this PR introduces

(E.g.: This pull request improves documation of area A by adding ....

Brief Changelog

(for example:)

  • The metadata is stored in the blob store on job creation time as a persistent artifact
  • Deployments RPC transmits only the blob storage reference
  • Daemons retrieve the RPC data from the blob cache

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #1614 (ecd23a1) into main (e9062fc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1614   +/-   ##
=======================================
  Coverage   19.60%   19.60%           
=======================================
  Files         242      242           
  Lines       32279    32279           
=======================================
  Hits         6329     6329           
  Misses      24792    24792           
  Partials     1158     1158           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9062fc...ecd23a1. Read the comment docs.

@nghuyenthevinh2000
Copy link
Member Author

nghuyenthevinh2000 commented May 28, 2022

currently, it faces a weird problem of: interfacetype only supports messages with exactly one oneof declaration

here is where the error comes from:

https://github.com/regen-network/cosmos-proto/blob/master/interfacetype/interfacetype.go#L56

Downloading gocosmos binary from cosmos will solve this: go get github.com/cosmos/gogoproto/protoc-gen-gocosmos 2>/dev/null

@ValarDragon
Copy link
Member

Is this different then whats going on in #1589

@faddat
Copy link
Member

faddat commented May 28, 2022

hi :) I thinik that you might be able to use cosmos-proto to resolve this. Maybe that's a 46 thing though?

@nghuyenthevinh2000
Copy link
Member Author

oh, it is the same, I will move over that issue

@nghuyenthevinh2000
Copy link
Member Author

nghuyenthevinh2000 commented May 31, 2022

@alexanderbez ser, can you check over .yaml files that I have written here as well as new_protocgen.sh

the generation is running

  1. protoc-gen-gocosmos in here instead of regen-network one: https://github.com/cosmos/gogoproto/tree/master/protoc-gen-gocosmos.
  1. what do you think about trying to remove regen-network related proto (deprecated) since cosmos/gogoproto can all provide them?

@nghuyenthevinh2000 nghuyenthevinh2000 marked this pull request as ready for review June 1, 2022 06:22
@nghuyenthevinh2000 nghuyenthevinh2000 requested a review from a team June 1, 2022 06:22
@alexanderbez
Copy link
Contributor

@nghuyenthevinh2000 how does this differ from my PR? I'd rather merge my PR (removing pulsar/ORM stuff) as it's a direct drop-in from the SDK.

@nghuyenthevinh2000
Copy link
Member Author

nghuyenthevinh2000 commented Jun 1, 2022

@nghuyenthevinh2000 how does this differ from my PR? I'd rather merge my PR (removing pulsar/ORM stuff) as it's a direct drop-in from the SDK.

  1. Firstly, I want to say that I understand your feelings.
  2. Secondly, direct from the SDK is not fit to Osmosis. I wil explain why:
  • buf.gen.gogo.yaml lacks protoc-gen-doc which is needed to generate proto-docs.md. Through my experience and understanding of buf.gen.yaml, I have added protoc-gen-doc to buf.gen.yaml with strategy "all". This is not present in buf.gen.gogo.yaml of the SDK.
  • My customization also consider "make proto-gen". tendermintdev/sdk-proto-gen:v0.2 docker image has only old buf which doesn't have "buf mod". Therefore, I have to locate the correct image that supports higher buf version and find out that tendermintdev/sdk-proto-gen:v0.3 docker is the only one that fits. v0.4 and above seems to have removed protoc-gen-doc.
  • Understanding of buf.gen.yaml "strategy" has allowed me to write significantly shorter and easier to read protocgen.sh
  1. As a buddhism practitioner myself, I want to promote cooperation and include your commits into this pr or commit into your pr. However you like it.

@alexanderbez
Copy link
Contributor

To be honest, I'd rather stay in line with what the SDK has. This includes using 0.7 proto version.

@nghuyenthevinh2000
Copy link
Member Author

nghuyenthevinh2000 commented Jun 1, 2022

To be honest, I'd rather stay in line with what the SDK has. This includes using 0.7 proto version.

I am a constant learner and I would love to hear your solution in generating proto-docs.md

So, the 0.7 proto version doesn't have protoc-gen-doc, this results in inability to generate proto-docs.md
Screenshot from 2022-06-01 22-05-16

And be frank with you, I don't know how to deal with the missing of protoc-gen-doc so I have to resort to v0.3. It has both protoc-gen-doc and buf version of 1.0.0-rc10, which supports "buf mod".

I would love to create a new tendermintdev/sdk-proto-gen:v0.7 with you that includes protoc-gen-doc.

@alexanderbez
Copy link
Contributor

I'm not sure I follow? Proto docs were and still are generated via make proto-gen.

@nghuyenthevinh2000
Copy link
Member Author

I'm not sure I follow? Proto docs were and still are generated via make proto-gen.

Yes, sir, you are correct.

proto docs are generated due to protoc-gen-doc.

In current "make proto-gen", it uses tendermintdev/sdk-proto-gen:v0.2 which includes protoc-gen-doc in go/bin.
Screenshot from 2022-06-01 22-24-58

It is the reason why proto docs are generated via "make proto-gen"

However, in tendermintdev/sdk-proto-gen:v0.7, protoc-gen-doc is removed. You can check and will not see protoc-gen-doc in go/bin.
Screenshot from 2022-06-01 22-05-16

Intially, I also use v0.7 but fail to generate proto docs due to missing protoc-gen-doc.

That is why I resort to v0.3. It has both protoc-gen-doc and buf version of 1.0.0-rc10 (which support "buf mod update")
Screenshot from 2022-06-01 22-29-37

@ValarDragon
Copy link
Member

ValarDragon commented Jun 2, 2022

I am in favor of only doing this upgrade when we switch to an SDK version that does it as well / changing things upstream in the SDK.

I would like everything around build process / proto generation to be as standard as possible, despite costs this may induce relative to the ideal. Maintaining alternate protobuf generation doesn't seem like a great reward vs maintenance payoff to me.

I currently build proto-docs via make proto-all and whatever thats doing internally.

Thanks for the interest in changing things here! I'd personally prefer closing this though / making isolated issues / PR's for discussing further problems we may be having / if theres a setup problem your having.

@nghuyenthevinh2000
Copy link
Member Author

I am in favor of only doing this upgrade when we switch to an SDK version that does it as well / changing things upstream in the SDK.

I would like everything around build process / proto generation to be as standard as possible, despite costs this may induce relative to the ideal. Maintaining alternate protobuf generation doesn't seem like a great reward vs maintenance payoff to me.

I currently build proto-docs via make proto-all and whatever thats doing internally.

Thanks for the interest in changing things here! I'd personally prefer closing this though / making isolated issues / PR's for discussing further problems we may be having / if theres a setup problem your having.

love to hear opinions. I will close this one for further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants