-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
protobuf: normalize how protobuf files are generated #5396
Conversation
The relative paths option for protoc generators doesn't work well when it comes to dependencies. This simplifies the code generation to avoid using `go generate` and to use one global command for protoc generation. This is similar to docker/buildx#2713 since the same problems with code generation occur here too. Signed-off-by: Jonathan A. Sternberg <[email protected]>
0x0a, 0x07, 0x53, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x18, 0x05, 0x20, 0x01, 0x28, 0x09, 0x52, | ||
0x07, 0x53, 0x65, 0x73, 0x73, 0x69, 0x6f, 0x6e, 0x12, 0x1a, 0x0a, 0x08, 0x46, 0x72, 0x6f, 0x6e, | ||
0x74, 0x65, 0x6e, 0x64, 0x18, 0x06, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x46, 0x72, 0x6f, 0x6e, | ||
0x74, 0x65, 0x6e, 0x64, 0x12, 0x57, 0x0a, 0x0d, 0x46, 0x72, 0x6f, 0x6e, 0x74, 0x65, 0x6e, 0x64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the meaning change in these binary Descriptors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is related to the file path changing. It affects the descriptor for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For finding the exact place, I believe the change that happens is for this field: https://github.com/protocolbuffers/protobuf/blob/be84e6b4b81b5b64158305020b0e75fc9e6bd3a5/src/google/protobuf/descriptor.proto#L98
Since we're not using relative file paths anymore, the "name" field gets changed to the full module path. The reason why I had to make this change originally was due to errdefs.proto
showing up in multiple file trees. buildx had an errdefs.proto
at github.com/docker/buildx/controller/errdefs/errdefs.proto
and buildkit had one at github.com/moby/buildkit/solver/errdefs/errdefs.proto
. Since both of these were errdefs.proto
from the perspective of protobuf, it was unhappy that there were two files with the "same path" that conflicted with each other.
Another reason is because of indirect imports. When we import wire.proto
from fsutil
it imports from stat.proto
. Since we changed fsutil to use the full import path instead of the relative one, that also affects all downstream dependencies. The reason why we had to change that was because some of the downstream dependencies were already using the full paths. It ended up being really difficult to continue using the relative paths.
The relative paths option for protoc generators doesn't work well when it comes to dependencies. This simplifies the code generation to avoid using
go generate
and to use one global command for protoc generation.This is similar to docker/buildx#2713 since the same problems with code generation occur here too.