update proto generation and testing pipelines #358
Conversation
williambanfield
requested review from
ebuchman,
josef-widder,
konnov and
milosevic
as code owners
proto/tendermint/abci/types.proto
Outdated
| @@ -1,8 +1,6 @@ | |||
| syntax = "proto3"; | |||
| package tendermint.abci; | |||
|
|
|||
| option go_package = "github.com/tendermint/tendermint/abci/types"; | |||
There was a problem hiding this comment.
what the new process here? generate in spec then move over?
I think for go this is required? (I can't remember exactly)
There was a problem hiding this comment.
when this is imported into Tendermint, will it not cause package issues?
package tendermint_abci
There was a problem hiding this comment.
whoops, never mind, I read the other PR now.
The only downside of doing it this way is, users of our photo files will need to append option go_package as well, this could cause confusion. Before it was plug and play.
There was a problem hiding this comment.
Unfortunately, this is really a downside of hosting the protos in the spec repo in general. If users were to try to generate these protos themselves using the old option, they would have created protos with import paths that do not match their local module name. As a result of this, they would have two options, both of which are not ideal.
Option 1. Rewrite the import paths locally, either before or after proto generation so that the paths match the local module. This is effectively what we are changing Tendermint to do.
Option 2. Shove the protos into the vendor directory, where packages are allowed to have a name that does not match the module name, but is also a directory that go programmers frequently cede to the go mod tooling to manage.
Neither option is great, and I think we can all agree that using the module name github.com/tendermint/tendermint in this package is strictly incorrect. If this solution causes serious headache for users who are attempting to vendor these protos into their local vendor directory, we can reconsider, but it also seems odd to me that a user would do that and not simply import the generated protos that exist within the Tendermint repository instead since they are effectively equivalent.
There was a problem hiding this comment.
The issue is that when the user imports the proto file into there proto file and generate their files the go import will be set to something else that isn't actually importing a file. This is how it is currently done for all go projects.
It's fine if we break this, wanted to make note of it mainly
There was a problem hiding this comment.
Makes sense, do you know of people doing this?
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
This pull request updates the `protocgen.sh` script to insert the `go_package` option to all of the downloaded proto files. A related pull request into the spec repo removes this options from the .proto files: tendermint/spec#358 This pull requests, along with the related spec PR, aim to move the creation of the `tendermintdev/docker-build-proto` container into the spec repo. This change also relies on several fixes to that container that are made in the PR into the spec repo.
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
…dermint#7269) This pull request updates the `protocgen.sh` script to insert the `go_package` option to all of the downloaded proto files. A related pull request into the spec repo removes this options from the .proto files: tendermint/spec#358 This pull requests, along with the related spec PR, aim to move the creation of the `tendermintdev/docker-build-proto` container into the spec repo. This change also relies on several fixes to that container that are made in the PR into the spec repo.
This pull request updates the `protocgen.sh` script to insert the `go_package` option to all of the downloaded proto files. A related pull request into the spec repo removes this options from the .proto files: tendermint/spec#358 This pull requests, along with the related spec PR, aim to move the creation of the `tendermintdev/docker-build-proto` container into the spec repo. This change also relies on several fixes to that container that are made in the PR into the spec repo.
This pull request aims to make it possible to generate, format, and lint the protos within this repo.
To accomplish that end, the Dockerfile containing common tools for building the tendermint protos has been moved into this repository. A related github workflow for building that dockerfile has been added to this repo (and removed from the tendermint repository). Additionally, a
buf.yamlandbuf.gen.yamlhave been added for working with thebuftool that we use for compiling our protos. A./third_partydirectory with the gogoproto proto file has been added to the root of the repo so that the proto compiler can find this dependency.The
go_packagevariable has been removed from all of the protos. This variable was set togithub.com/tendermint/tendermint/{PACKAGE}which does not make any sense since this is not the tendermint repository. This variable is now set when the protos are built within the tendermint repo. This does present the downside that users who wish to build this protos in go will also need to manually add this option. The community of users doing that seems likely to be small, and we can provide support to them with the scripts that are added in: tendermint/tendermint#7269.