Skip to content

[dev] protoc-gen-go: reorganize, fix testdata directory #520

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

Merged
merged 5 commits into from
Feb 21, 2018

Conversation

neild
Copy link
Contributor

@neild neild commented Feb 20, 2018

"make test" now compiles all the source files in testdata/ using the
protoc-gen-go in the current working directory and compares the output
to golden versions, "make regenerate" rebuilds the golden files.

Add a go_package option to each proto source file. Put the sources for
each package in the proper directory.

Add a golden_test.go which arranges to compile each source file using
the compiler in the working tree.

This does not touch the content of any of the sources in testdata/
other than to add go_package options and fix up import paths.

Change-Id: Iea5bef9bba626116b8ce5ea136a15f3cff4f5bcc

"make test" now compiles all the source files in testdata/ using the
protoc-gen-go in the current working directory and compares the output
to golden versions, "make regenerate" rebuilds the golden files.

Add a go_package option to each proto source file. Put the sources for
each package in the proper directory.

Add a golden_test.go which arranges to compile each source file using
the compiler in the working tree.

This does not touch the content of any of the sources in testdata/
other than to add go_package options and fix up import paths.

Change-Id: Iea5bef9bba626116b8ce5ea136a15f3cff4f5bcc
@neild neild requested a review from dsnet February 20, 2018 22:12
Change-Id: I4dcf2ed1c9edf713bdf20cdb844599921e36566c
…nore.

Change-Id: I11370c095fcb2638ef5a050e6a843517107da107
Change-Id: I57a5edf2e9e0c4f70efc4d4fa97b79cd97925757
t.Fatal(err)
}

// Compile each package, using this binary as protoc-gen-go.
Copy link
Member

Choose a reason for hiding this comment

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

Cute.

return nil
}

//
Copy link
Member

Choose a reason for hiding this comment

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

Spurious comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where that came from; fixed.

Change-Id: I9172ec10cc25d2e966ccd8d2af94130a546d2792
@neild neild merged commit b322e49 into golang:dev Feb 21, 2018
@neild neild deleted the dev.alias branch February 21, 2018 01:06
@dsnet
Copy link
Member

dsnet commented Feb 21, 2018

FYI, next time use "squash and merge" instead of just "merge" so that it doesn't pollute the commit history with inconsequential commits like 055d7b0.

If you think it's a good path forward, I would like to move towards using Gerrit which provides much better granularity in code review and also ensures the one-commit-per-CL property. Also the new Gerrit<->GitHub tool does a good job enabling users to stay with GitHub pull requests if they so desire.

@neild
Copy link
Contributor Author

neild commented Feb 21, 2018

+1000 to using Gerrit.

@awalterschulze
Copy link

Just as you join the real github community you retreat back to Gerrit again.
Please consider staying on github and having discussions with your users.

@dsnet
Copy link
Member

dsnet commented Feb 21, 2018

I think there is a misunderstanding.

For some history, the initial version of Go protobuf was written by members of the Go team that have long since moved on to other things. I admit the stewardship of this repo has been pretty poor for quite some time, and I apologize for that. I want to be more involved in the maintenance and development of this project, but have had my attention pulled in many directions. However, there is a good indication that my involvement here will increase significantly in the near future.

I am aware that a portion of the community that uses Go protobufs are users of gogo/protobuf and that there are some negative sentiment in that community regarding the interoperability of golang/protobuf and gogo/protobuf (e.g., gogo/protobuf#386 or depends-on-golangprotobuf). I understand your frustration. Nearly all of interoperability issues can be traced back to fundamentally issues with the proto API, which is captured in #364. I care about fixing the interopability issue and it is personally my highest priority item in protos to address. I also care about engaging with the external community on how to improve Go protobufs. When I have something to concrete to show for how to fix this mess, I value your opinion and that of the community.

Regarding the "real" community: protobufs are a technology originally developed by Google and there is a very large community inside Google as well. As a maintainer of this project, I care about both the internal and external community. Inside Google, there are a lot of legacy protobuf features that were never released to the external world (to their benefit as Google has long considered these esoteric features a mistake). However, we do have to continue maintaining these "features" internally. Moving primary development to GitHub was not an easy decision for us since it required us to maintain those legacy features as a set of internal patches on top of the external repo. However, we did it because we valued being able to develop in a more open environment.

That being said, we are not "retreating" anywhere. The code review system inside Google is not Gerrit. Gerrit is only a code-review system. We have no intention to make the internal code repository inside Google the source-of-truth again. Development is currently and will continue to be done in the open source world. However, what I personally would like to see changed, is the switch to a more thorough code-review tool, which is Gerrit. To be clear, doing so will not prevent users who like to use GitHub pull-requests to continue doing so. There is a tool that synchronizes reviews between GitHub and Gerrit (e.g., see golang/go PRs).

Personally, I have used both GitHub pull requests (which I grew up on and have seen improve over time) and also Gerrit and find Gerrit overall superior tool even if it has a steeper learning curve. Using both Gerrit and GitHub PRs is a good middle ground to meet developers where they desire to develop.

Again, to re-emphasize, GitHub PRs would still be accepted and we desire to expand the community involvement.

@awalterschulze
Copy link

Thank you for the transparency into the patches inside Google. I know that maintaining a diff of a project is a lot of work and I REALLY appreciate the effort of moving the primary version into the public. Does not sound like it was easy or will continue to be easy.

I have also used both Gerrit and github review and slightly prefer github.
I did not know about the sync tool though.
On the surface the tool looks like a good compromise.

Looking forward to working more with you and nield and trying to resolve the incompatibilities :)

@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants