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 protobuf deps #757

Closed
wants to merge 1 commit into from
Closed

Add protobuf deps #757

wants to merge 1 commit into from

Conversation

pinosu
Copy link
Contributor

@pinosu pinosu commented Feb 14, 2022

Add dependencies to buf.yml
Remove third_party folder, since dependencies are fetched using the deps in buf.yml
Update Makefile
Update protocgen.sh script

@pinosu pinosu requested a review from alpe as a code owner February 14, 2022 14:09
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #757 (ee67dd7) into master (f043f19) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
+ Coverage   58.47%   58.51%   +0.03%     
==========================================
  Files          49       49              
  Lines        5828     5828              
==========================================
+ Hits         3408     3410       +2     
+ Misses       2170     2169       -1     
+ Partials      250      249       -1     
Impacted Files Coverage Δ
x/wasm/keeper/keeper.go 87.90% <0.00%> (+0.35%) ⬆️

Copy link
Member

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Excellent work. 💯
I expect that we need to pin the sdk protobuf versions to the sdk version that we use in go.mod. Can you :eye add the linked doc and add this to this PR, please?
Bonus points for some minimal doc in proto/readme how to do it.

@@ -10,7 +10,7 @@ SIMAPP = ./app

# for dockerized protobuf tools
DOCKER := $(shell which docker)
BUF_IMAGE=bufbuild/buf@sha256:9dc5d6645f8f8a2d5aaafc8957fbbb5ea64eada98a84cb09654e8f49d6f73b3e
BUF_IMAGE=bufbuild/buf:1.0.0-rc12
Copy link
Member

Choose a reason for hiding this comment

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

👍

google.golang.org/genproto v0.0.0-20211208223120-3a66f561d7aa
google.golang.org/grpc v1.43.0
google.golang.org/genproto v0.0.0-20220211171837-173942840c17
google.golang.org/grpc v1.44.0
Copy link
Member

Choose a reason for hiding this comment

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

Can you run go mod tidy to clean up the files?

- buf.build/cosmos/cosmos-proto
- buf.build/cosmos/cosmos-sdk
- buf.build/cosmos/gogo-proto

Copy link
Member

Choose a reason for hiding this comment

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

So nice to see all the files removed. To achieve the same version "safety" for our dependencies, we need to pin them. I found this in the doc: https://docs.buf.build/tour/add-a-dependency#pin-your-dependencies for doc

I am not sure though if the files are available for the sdk v0.45.1 already.

x/wasm/types/types.pb.go Show resolved Hide resolved
x/wasm/types/query.pb.gw.go Show resolved Hide resolved
@alpe
Copy link
Member

alpe commented Mar 9, 2022

This PR is pending as we need to find the "right" sdk protobufs versions to match our v0.45.x version

@ethanfrey
Copy link
Member

This PR is pending as we need to find the "right" sdk protobufs versions to match our v0.45.x version

Can you explain this more? Seems like an issue with the buf publishing system as used by the cosmos-sdk?

Maybe @marbar3778 could help on this as he has been working to get more adoption of buf.build?

@alpe
Copy link
Member

alpe commented Mar 11, 2022

Without "pinning" the protobufs to a fix version or tag, we would link to a moving target and could not have fully reproducible code-generations. The sdk publishes versions with the git hash as label (https://buf.build/cosmos/cosmos-sdk/history/main). This is great but seems to be for master only. We could not find the matching protobufs for the v0.45.1 hash 2646b474c7beb0c93d4fafd395ef345f41afc251

@marbar3778 it would be great if you could help with pushing the protobufs for this version and automating this step to have them available for future tags as well.

@ethanfrey
Copy link
Member

What is the state of this PR?

If it is done and makes better protobuf generation, let's get it updated and merged. Or close if not needed

@pinosu
Copy link
Contributor Author

pinosu commented Apr 27, 2022

We are still blocked for the same reason @alpe has explained above.

@ethanfrey
Copy link
Member

We are still blocked for the same reason @alpe has explained above.

Ah, now I understand that comment.

@ethanfrey
Copy link
Member

@marbar3778 could you look into the buf.build publishing of the sdk (so they publish the actual releases, like v0.45.4) not just main branch?

Or pass this off to someone working on that in the core team?

@tac0turtle
Copy link
Contributor

@marbar3778 could you look into the buf.build publishing of the sdk (so they publish the actual releases, like v0.45.4) not just main branch?

Or pass this off to someone working on that in the core team?

we publish each time since proto version is not tied to software versioning. On buf you will see the sdk protofiles have v1beta1 and v1 tags.

@pinosu
Copy link
Contributor Author

pinosu commented May 19, 2022

Hi @marbar3778 , it was nice to meet you at gateway conference ! We didn't have the chance to discuss about this PR. I have now created a PR in cosmos-sdk, and I think this will solve the issue we are having when trying to include cosmos-sdk protobufs from BSR.
Please let me know if it makes sense for you.

@pinosu
Copy link
Contributor Author

pinosu commented Oct 3, 2022

Closing in favor of #1030

@pinosu pinosu closed this Oct 3, 2022
@alpe alpe deleted the add-protobuf-deps branch May 10, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants