Closed awalterschulze closed 2 years ago
I would really hate to see this project die and would love to see parity with the new Proto APIv2. I would like to help contribute as much as I can. Do you think before the existing core contributors move on, can create a milestone or set of issues needing to be completed to be compatible with the latest v2 updates?
I share the same sentiment. I would like to contribute and help organize gogoproto. That being said I am unfamiliar with the inner workings of gogoproto and I am a junior developer, but I am willing to learn and work with others to help this project live on.
The first task is to identity the current incompatibilities with APIv2. Just a quick look at the issue tracker gives me:
There will probably be more, but it would be good to first get an idea of the exact shortcomings and necessary fixes. Assuming no bigger org steps up to take over, I think the short term goal is 1.4.x compatibility. If ownership can be transferred, there could be talk of a GoGo protobuf APIv2.
Would love to take part of this project @johanbrandhorst, @awalterschulze
Hello @awalterschulze (and other gogo protobuf maintainers), thanks for opening up this discussion!
Would you be able to give a bit more details on how would this new ownership would be ideally structured?
Thank you. These are great questions.
Are you trying to find a new home to step down as the leaders of this project?
Yes and ideally a new home, where the employer(s) or team(s) acknowledges and gives those employee(s) at least some time to work on the project.
Are you looking to share the responsibility with a new group of maintainers?
No. All our circumstances have changed so much that this won't be possible.
Would the current maintainers be interested in a sponsorship to spend some focus time on gogo protobuf?
I know this is a No for me and Johan and highly probably a no for Donald and Jacques.
We recently did some work to port a project from GoGo Protobuf to the v2 API. Aside from gogoproto.nullable
(which we use quite a bit, in particular with WKTs), the blockers we identified were custom names, struct tags, and native well-known types.
Requests for these features are tracked here on the Go Protobuf repo:
go_name
: https://github.com/golang/protobuf/issues/555The v2 reflection API might make implementing some of these easier than forking the project. If these features are important to you, I’d recommend weighing in on these issues.
Reporter of #686 here. At work we're heavily reliant on gogo/protobuf
for ergonomic gRPC APIs, and rewriting every enum and re-inventing a struct tag injector is not an option.
I tried to do some of the porting at that time, but it seems much of the machinery that the original maintainers used to track upstream is not public stuff (say, committed in this repo). On top of that it's generally hard to justify general open-source maintenance work when the department is under-staffed. We're watching this situation though, and I have just enough experience in compilers to review and debug code. If anyone steps up and decides to do the grunt work, I could probably spend a few weekends to help.
Does someone want to compile a list of issues in a comment here & then we can work on creating a milestone to work towards?
It seems there are a few people that have stated interest in keeping this repo alive. And work towards upgrading this repo.
Feel free to start with the ones linked already. We don't need a milestone, I don't think?
https://github.com/gogo/protobuf/issues/695 Is another one
where is the best place to ask questions in regards to updating? is there a chat somewhere I can join or is here the best....?
The #protobuf channel on Gophers slack is probably a good place to start.
I see many teams moving out of gogo. I considered the same for our project and run simple benchmark test to compared v1, v2, and gogo: https://github.com/alexshtin/proto-bench.
The results show that gogo is still ~2 times faster and v2 is actually slower than v1. We decided to stay on gogo. Unfortunately, we don't have resources to provide much help now but we might have them later.
I would love to see this project maintained and would offer to contribute as I can. We currently are maintaining a fork https://github.com/regen-network/protobuf for use in https://github.com/cosmos/cosmos-sdk. I have a few open PR's that we are using actively on our fork: #657, #658, and #426
A few of my colleagues working on Cosmos/Tendermint. @alexanderbez and @marbar3778 have chimed in here as well so we may have some limited bandwidth, but we likely wouldn't want to be the only org responsible for maintenance and upgrading to v2. If anyone else is interested let us know.
It seems from the comments that there are parallel efforts to upgrade the v2 implementation to support some of the gogo proto features, yet that implementation is slower. Either way, it would be great to see the community align on a path forward rather than splintering - either upgrading gogo to v2 or porting gogo features to v2.
- Struct tags: golang/protobuf#52
@ydnar after years of wishfully waiting for struct tags, it seems now that golang/protobuf has decided not to implement this feature :(
- Struct tags: golang/protobuf#52
@ydnar after years of wishfully waiting for struct tags, it seems now that golang/protobuf has decided not to implement this feature :(
Agreed, I’m disappointed. We kind of stirred the pot with golang/protobuf#1142, but I’m glad that the Go protobuf maintainers have at least clarified their position.
We’ve prototyped an internal tool that wraps protoc-gen-go
, intercepting the CodeGeneratorRequest
and CodeGeneratorResponse
, parses the Go source using the ast
and types
packages, and modifies the output before serialization.
I’ll share what we have when we have something working for (at least) our use cases.
@ydnar -- I think https://github.com/srikrsna/protoc-gen-gotag does exactly what you have described
I’ll share what we have when we have something working for (at least) our use cases.
Progress update: we now have a working protoc-gen-go-patch
tool that does what I described above. It uses the v2 protogen
package and go/ast
/ go/types
for most of the heavy lifting, and doesn’t require forking protoc-gen-go
. I hope to extract it as a separate public package sometime next week.
Currently supported options: go_tags
, go_name
, go_message_name
, go_enum_name
, and go_value_name
. TODOs include gRPC, oneof
support, and a patch/gogo
package for gogoproto.*
options that alias to the above.
More interesting features like GoGo’s WKTs like time.Time
are more complicated, but it’s a fun start.
@ydnar this sounds very promising. The TODO list as you describe should be highest priority. I hope that customtype is not necessary, but do you think casttype would be possible?
gogoproto.* options that alias to the above.
This sounds like an interesting migration story.
@ydnar could you give an example of how users specify the tags, names, etc?
Is it also using the protobuf extensions, like https://github.com/srikrsna/protoc-gen-gotag and gogoprotobuf or do you have a new method?
@ydnar could you give an example of how users specify the tags, names, etc?
Is it also using the protobuf extensions, like https://github.com/srikrsna/protoc-gen-gotag and gogoprotobuf or do you have a new method?
Exactly. It uses extensions without a package name to allow proto files to specify short option names. Here’s an example:
https://gist.github.com/ydnar/fbb49f92412ecd6bb895b36e90c9a707
I hope that customtype is not necessary, but do you think casttype would be possible?
Good question. Modifying a basic type in an AST should be straightforward. More than that will require surgery. The nullable
option will require changes: golang/protobuf#1142
Edit: @awalterschulze do you have any data on how often and how casttype
is used, and if users specify a fully-qualified package + type selector, or just a type?
Progress:
oneof
fields.enum
values when enum
is renamed.oneof
fields and synthesized types/methods when parent message
is renamed.patch/gogo
package to support GoGo Protobuf options like gogoproto.custom_name
as an alias for go_name
.Edit: @awalterschulze do you have any data on how often and how
casttype
is used, and if users specify a fully-qualified package + type selector, or just a type?
I used to use it this way -> https://github.com/moul/depviz/blob/cc631458b3262ff3e121fdd15b49ff42c57777bc/api/dvmodel.proto#L41
@moul
I don't have data, but I expect it would mostly be qualified as in your example.
I pushed an early build of protoc-gen-go-patch
here: https://github.com/alta/protopatch
YMMV, but it works for us. TODO: tests, GoGo shim, more features.
Update: https://github.com/alta/protopatch has a new—arguably less simple, but easier to extend—API:
go.message.options
— message options, which modify the generated Go struct for a message.go.field.options
(also aliased to go.options
) — message field options, which modify Go struct fields and getter methods.go.oneof.options
— oneof
field options, which modify struct fields, interface types, and wrapper types.go.enum.options
— enum
options, which modify Go enum types and values.go.value.options
— enum
value options, which modify Go const
values.import "patch/go.proto";
import "patch/go/message.proto";
import "patch/go/field.proto";
import "patch/go/enum.proto";
import "patch/go/value.proto";
message OldName {
option (go.message.options) = {name: 'NewName'};
int id = 1 [(go.field.options) = {name: 'ID'}];
}
enum Errors {
option (go.enum.options) = {name: 'ProtocolErrors'};
INVALID = 1 [(go.value.options) = {name: 'ErrInvalid'}];
NOT_FOUND = 2 [(go.value.options) = {name: 'ErrNotFound'}];
TOO_FUN = 3 [(go.value.options) = {name: 'ErrTooFun'}];
}
message ToDo {
int32 id = 1 (go.field.options) = {name: 'ID', tags: '`xml:"id,attr"`'}]
string description = 2 (go.field.options) = {tags: '`xml:"desc"`'}]
}
casttype
is more complex.@ydnar I gave a try to your https://github.com/alta/protopatch repo, it works very well and I like the syntax, the usage etc
fyi, for now, the only thing that prevents me to switch real projects is the lack of (gogoproto.stdtime) = true
, but the project is very promising :+1:
I have an initial spike on a GoGo compatibility shim, with support for one extension (gogoproto.customname
). Having gone through this exercise, I’m not sure the juice is worth the squeeze. A full GoGo shim has the potential to be larger than the rest of the package. Is it worth it to maintain, when a single find & replace could potentially suffice? I’m not sure.
https://github.com/alta/protopatch/pull/8
I’m curious—which GoGo extensions do folks value most? Which could you live without? Which are obsoleted or mitigated by APIv2?
I’m curious—which GoGo extensions do folks value most? Which could you live without? Which are obsoleted or mitigated by APIv2?
$ for repo in pathwar.land berty.tech/berty berty.tech/yolo ultre.me/calcbiz moul.io/depviz github.com/moul/engine; do echo $repo; cd ~/go/src/$repo; git grep gogoproto\\. | grep proto: | tr " " "\n" | grep gogoproto\\. | sed 's/.*(\(gogoproto\.[^)]*\))/\1/' | sort | uniq -c; echo; done
pathwar.land
89 gogoproto.customname
1 gogoproto.goproto_enum_prefix_all
6 gogoproto.marshaler_all
151 gogoproto.moretags
53 gogoproto.nullable
6 gogoproto.sizer_all
53 gogoproto.stdtime
6 gogoproto.unmarshaler_all
berty.tech/berty
93 gogoproto.customname
3 gogoproto.goproto_enum_prefix_all
2 gogoproto.jsontag
3 gogoproto.marshaler_all
3 gogoproto.sizer_all
3 gogoproto.unmarshaler_all
berty.tech/yolo
45 gogoproto.customname
1 gogoproto.marshaler_all
12 gogoproto.moretags
19 gogoproto.nullable
1 gogoproto.sizer_all
19 gogoproto.stdtime
1 gogoproto.unmarshaler_all
ultre.me/calcbiz
11 gogoproto.customname
4 gogoproto.marshaler_all
4 gogoproto.sizer_all
4 gogoproto.unmarshaler_all
moul.io/depviz
16 gogoproto.casttype
9 gogoproto.customname
1 gogoproto.equal_all
1 gogoproto.goproto_getters_all
1 gogoproto.goproto_registration
2 gogoproto.marshaler_all
49 gogoproto.moretags
9 gogoproto.nullable
1 gogoproto.populate_all
2 gogoproto.sizer_all
8 gogoproto.stdtime
2 gogoproto.unmarshaler_all
github.com/moul/engine
7 gogoproto.casttype
4 gogoproto.customname
5 gogoproto.enumdecl
251 gogoproto.enumvalue_customname
5 gogoproto.goproto_enum_prefix
5 gogoproto.goproto_enum_stringer
20 gogoproto.goproto_getters
1 gogoproto.goproto_getters_all
3 gogoproto.goproto_stringer
2 gogoproto.marshaler_all
14 gogoproto.nullable
6 gogoproto.protosizer_all
6 gogoproto.sizer_all
10 gogoproto.stdduration
3 gogoproto.stdtime
20 gogoproto.typedecl
2 gogoproto.unmarshaler_all
summary:
$ for repo in pathwar.land berty.tech/berty berty.tech/yolo ultre.me/calcbiz moul.io/depviz github.com/moul/engine; do cd ~/go/src/$repo; git grep gogoproto\\. | grep proto: | tr " " "\n" | grep gogoproto\\. | sed 's/.*(\(gogoproto\.[^)]*\))/\1/'; echo; done | sort | uniq -c | sort -n -r
251 gogoproto.enumvalue_customname
251 gogoproto.customname
212 gogoproto.moretags
95 gogoproto.nullable
83 gogoproto.stdtime
23 gogoproto.casttype
22 gogoproto.sizer_all
20 gogoproto.typedecl
20 gogoproto.goproto_getters
18 gogoproto.unmarshaler_all
18 gogoproto.marshaler_all
10 gogoproto.stdduration
6 gogoproto.protosizer_all
6
5 gogoproto.goproto_enum_stringer
5 gogoproto.goproto_enum_prefix
5 gogoproto.enumdecl
4 gogoproto.goproto_enum_prefix_all
3 gogoproto.goproto_stringer
2 gogoproto.jsontag
2 gogoproto.goproto_getters_all
1 gogoproto.populate_all
1 gogoproto.goproto_registration
1 gogoproto.equal_all
I can't say if it's a good idea to be 100% compatible with gogoproto (I prefer new protopatch usage)
The most important missing ones for me are:
gogoproto.stdtime
gogoproto.nullable
gogoproto.casttype
And a very cool one too:
gogoproto.goproto_enum_prefix_all
I can't say if it's a good idea to be 100% compatible with gogoproto (I prefer new protopatch usage)
Thanks!
The most important missing ones for me are:
gogoproto.stdtime
The new helper functions in latest release of protobuf-go
enable converting to/from standard time and duration. They’re great.
gogoproto.nullable
Unlikely to happen unless the protobuf-go
authors change an IsNil
check on sub-messages: https://github.com/golang/protobuf/issues/1142
gogoproto.casttype
I think this is possible with some work.
gogoproto.typedecl
Interesting. How do you use it?
And a very cool one too:
gogoproto.goproto_enum_prefix_all
How would you imagine this working with the protopatch
API?
gogoproto.stdtime
The new helper functions in the latest release of protobuf-go enable converting to/from standard time and duration. They’re great.
I really need a time.Time
type and not helpers, because gorm (and probably a lot of other reflection-based libraries) doesn't understand that a struct of two fields is actually a timestamp; my current fallback is to use int64 Unix timestamps for DB models
no gogoproto.stdtime
-> no proto.Timestamp
and proto.Duration
at all in some library combinations
gogoproto.typedecl
Interesting. How do you use it?
I'm not the author of the code, but you can look here:
https://github.com/bblfsh/bblfshd/blob/master/daemon/protocol/generated.proto
I just updated my previous command to remove it as the "most important missing things" section since I don't know if it's important :)
@awalterschulze hello, sorry for mentioning – maybe pin this issue to be visible in issues list?
P.S. Many thanks for work on this project to you and all other maintainers.
Good idea. It seems the issue is already pinned. I tried to repin it.
@awalterschulze Sorry to hear your moving on! Thanks for the great project and good luck with your next venture. Gogo was a pleasure to use and really helped fix a lot of the usability problems with protobuf. I have always been impressed with how your maintainership moved forward on usability where the upstream did not.
Thank you @stevvooe those are very kind words and much appreciated. You also helped this project a lot, first external contributor, where you added enumvalue_customname
and enum_customname
pushing the initiative for gogoprotobuf to generate golint compatible code. It was really great to work with you.
Would it be possible to get a label/milestone tracking the work? I would love to pick up a bugfix here or there when possible, but sadly can't dedicate time to maintenance.
Thank you so much for your efforts maintaining this, gogoproto has been an incredible boon to the Protobuf and Go ecosystem.
I've added an apiv2 label (https://github.com/gogo/protobuf/issues?q=is%3Aissue+is%3Aopen+label%3Aapiv2), if anyone is interested in starting to look through the issues. Fair warning, the issues will run quite deep.
@awalterschulze
Hi,
I can start maintain gogo-pb, what should I do first?
I'm maintainer of json-iterator, go-toml, beego etc. now and lots of libraries in my company dependence on gogo-pb .
I think I can maintain gogo-pb in long term.
@AllenX2018 Hello! :sweat_smile:
I'm glad to hear you want give a hand, but I think you are likely misunderstanding between maintain
and contribute
.
It's not a problem, because the most important thing is that you want to become a maintainer. So, the first thing that is you want to fix all of the issues by PR
s and whatever make it works.
Hi @AllenX2018 thank you for offering. Just to make sure we are on the same page. You are offering to take over maintenance of gogoprotobuf in the long term? In other words, take over leadership of the project. Or are you offering to contribute, by addressing some issues?
If you are offering to take over and lead, what would be your plan? Do you have the backing of your company, because this will probably require more work, than just out of office hours? Or do you have a partner or two in this, so you can share the load? What do you think would be most important to fix first? etc.
If you are offering to contribute by fixing some issues. Then unfortunately this is not what this issue is about. We need someone to review other peoples' contributions (pull requests), reply to issues opened, create a plan to handle proto2, etc. This is not a small amount of work.
I think it is important that the new owner(s) have their company rely on this project and back the person(s) maintaining it.
I might be wrong, this is my first time handing over such a big project, so I am also learning.
I took the time to re-implement the top 3 extensions needed by our projects, in the form of protobuf-gogogo (pardon the name). Note this is very preliminary work without testing infrastructure set up, don't use in (your) production yet.
The extensions implemented:
gogoproto.moretags
gogoproto.jsontag
gogoproto.enumvalue_customname
gogoproto.stdtime
is unneeded with APIv2 IMO, because the Timestamp
WKT has convenience methods that don't return err
, the ergonomics was hugely improved.
given the result vitess.io showed in this blob post, I would say gogoprotobuf has a good contender for a replacement.
given the result vitess.io showed in this blog post, I would say gogoprotobuf has a good contender for a replacement.
Agreed, absolutely great job @vmg Unfortunate to loose some features, but having them in gogoprotobuf added a huge maintenance burden that was unfortunately not sustainable. vtprotobuf has a sustainable maintenance path and very impressive speed improvement provided by memory pooling. I really enjoyed reading the well written post.
Thank you! As pointed on the post, there are important Gogo features that we cannot support (most notably: non-nullable fields, which make a significant improvement in GC stress). In exchange, we do support ProtoBuf APIv2 and hopefully we're gonna have an easy time keeping the project up to date with the changes in upstream ProtoBuf. Please feel free to check out the new codegen and report bugs in our repository!
@vmg
Maybe it is okay to not support nullable, given proto3?
It should be easy to add: gostring
for debugging, populate
for testing.
But this could also maybe be covered by goderive
https://github.com/awalterschulze/goderive/tree/master/example/plugin/gostring
I think the only hard feature, I would consider supporting is casttype
.
How hard would that be with APIv2?
@vmg Thank you for a great post, and sharing the tool! I have a question though
we cannot support
If community comes with MRs to add some missing pieces, would you consider adding them, or you only add what you can support, sorry?
@tandr: we're always open to new features, but as the blog post explains, supporting a feature like optional nullable fields would require forking all of protoc-gen-go
, which is something we don't want to do (even if somebody were to send a PR) because the long term maintenance cost is very high.
I write to you on behalf of the GoGo Protobuf project, which has long been supported by a small group of dedicated developers taking time out of their lives to keep the project up to date and add new features, to the benefit of all users. Unfortunately, the personal circumstances of the maintainers have changed and we are no longer able to keep up with the issues and feature requests that naturally crop up for a popular open source project.
In particular, the recent golang/protobuf release 1.4.x (AKA APIv2: https://blog.golang.org/protobuf-apiv2), has created a big chunk of work required to be compatible with the new world of Go protobufs. While longer term this work will likely make gogo/protobuf much more compatible with the rest of the ecosystem, there is currently no one to take on this piece of work, and users are living in uncertainty around the future of the project.
We are therefore reaching out to industry users such as yourself to ask for help with maintaining the project. Without external help, it is likely that the GoGo Protobuf project will be discontinued. We know there are many organisations out there that use the project, so we're hoping that one or more of them would step up to take on this maintenance.
Please let us know if you are a user that can take on (co)ownership of this project. It is as simple as commenting on this issue.
Sincerely,
@awalterschulze and @johanbrandhorst
From Walter:
I want to thank @jmarais and @donaldgraham for doing a great job stepping up to take on maintenance of GoGo Protobuf over the past 2 years. I want to also thank @johanbrandhorst for developing and maintaining the GRPC interoperability. I am very lucky and proud to call you all my friends.