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

Specify version for proto-gen Docker image #2791

Merged
2 commits merged into from
May 25, 2022
Merged

Specify version for proto-gen Docker image #2791

2 commits merged into from
May 25, 2022

Conversation

jprupp
Copy link
Contributor

@jprupp jprupp commented May 21, 2022

Fixes being unable to compile protobufs using:

make gen-proto

@ghost
Copy link

ghost commented May 23, 2022

Is there a test that is run to ensure the generated code is compatible with the old one? Or can we run such a test? I'm asking since the generated code in x/clp/types/querier.pb.gw.go differs. Though I cannot tell if it's relevant or not.

And I suppose the generated code should be committed as well

@jprupp
Copy link
Contributor Author

jprupp commented May 23, 2022

I didn't put the generated file in this change on purpose, as I wanted to only fix the make proto-gen target, letting the owner of the proto file assess whether it works as intented.

But I think you are right @sifag, and we better rip that bandaid in one go.

I did some digging, and found that @timlind committed a new version of the affected file querier.pb.gw.go to the repository recently:

efe30b5

That’s understandable, since that file was affected by PMTP changes.

Now, I wonder how @timlind compiled the file, as if he used the make protoc-gen target, it would fetched the Docker Hub proto-gen image with the latest tag, which I would assume to be this one, which was the latest tagged version at the time:

https://hub.docker.com/layers/sdk-proto-gen/tendermintdev/sdk-proto-gen/v0.7/images/sha256-a5efd2bf712a1e5c1a20cd16eb64a549d58089ff0b2988eaf8ba25223bf0203b?context=explore

@timlind can you tell me how you managed to compile these protobuf files?

Greetings

@jprupp
Copy link
Contributor Author

jprupp commented May 23, 2022

By the way, I reviewed my assumption that the latest tag in Docker Hub points to the latest version tag that pushed: it doesn’t. it just another tag that is set by the developer, and calling it latest is just a convention.

Based on the date that the latest image was deployed for proto-gen:

https://hub.docker.com/layers/sdk-proto-gen/tendermintdev/sdk-proto-gen/latest/images/sha256-4a749a2800dbdfdd1ca4d0394bc4f59c0456f7374257704f074406d6eba3653d?context=explore

It probably doesn't point to the code in version tag v0.7 as I assume, because that one is much more recent than the last push to latest, which happened two hours after tag v0.6 was pushed.

But the code pushed to latest is likely not the same as the code pushed to v0.6 either, as sizes and hashes are all different.

@jprupp
Copy link
Contributor Author

jprupp commented May 23, 2022

Digging deeper, I found that our use of buf protoc is deprecated by the Buf tool:

https://github.com/bufbuild/buf/releases/tag/v1.0.0

Remove buf protoc. This was a pre-v1.0 demonstration to show that buf compilation
produces equivalent results to mainline protoc, however buf is working on building
a better Protobuf future that provides easier mechanics than our former protoc-based
world. buf protoc itself added no benefit over mainline protoc beyond being considerably
faster and allowing parallel compilation. If protoc is required, move back to mainline protoc
until you can upgrade to buf. See bufbuild/buf#915 for more
details.

So, we need to move away from this. Already Cosmos SDK has. I'll see about this.

@jprupp
Copy link
Contributor Author

jprupp commented May 23, 2022

The reason that the generated Protocol Buffers Go code that @timlind uploaded to the repository differs from that which is generated via make proto-gen is that he used the underlying directly against his own installed versions of buf and protoco, and didn’t use the Docker and make proto-gen.

@jprupp jprupp force-pushed the feature/fix-gen-proto branch from 356e67a to fad0cc9 Compare May 24, 2022 19:36
@ghost ghost merged commit c383fcc into develop May 25, 2022
@ghost ghost deleted the feature/fix-gen-proto branch May 25, 2022 14:13
This pull request was closed.
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