protobuf: remove gogoproto#2713
Merged
Merged
Conversation
jsternberg
force-pushed
the
gogoproto-remove
branch
from
9a4812a to
993e9a8
Compare
jsternberg
force-pushed
the
gogoproto-remove
branch
from
993e9a8 to
1e2cd95
Compare
github-actions
Bot
added
area/bake
area/build
area/cli
area/controller
area/hack
area/tests
labels
jsternberg
changed the title
jsternberg
force-pushed
the
gogoproto-remove
branch
4 times, most recently
from
5c304ae to
3cb79af
Compare
crazy-max
reviewed
Sep 27, 2024
| go 1.21.0 | ||
| go 1.22.0 | ||
|
|
||
| toolchain go1.22.2 |
Collaborator
Author
There was a problem hiding this comment.
I'm honestly not even sure what added this.
jsternberg
force-pushed
the
gogoproto-remove
branch
from
3cb79af to
3af7537
Compare
Removes gogo/protobuf from buildx and updates to a version of moby/buildkit where gogo is removed. This also changes how the proto files are generated. This is because newer versions of protobuf are more strict about name conflicts. If two files have the same name (even if they are relative paths) and are used in different protoc commands, they'll conflict in the registry. Since protobuf file generation doesn't work very well with `paths=source_relative`, this removes the `go:generate` expression and just relies on the dockerfile to perform the generation. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
gogoproto-remove
branch
from
3af7537 to
b35a0f4
Compare
tonistiigi
approved these changes
Oct 2, 2024
Member
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
Some follow-ups but I think we can bring this in.
| return nil, err | ||
| } | ||
| if stat.Size() > 512*1024 { | ||
| if proto.Size(stat) > 512*1024 { |
Member
There was a problem hiding this comment.
This is meant to be file size. We also should probably increase the limit (in a different PR).
| flags.BoolVarP(&options.all, "all", "a", false, "Include internal/frontend images") | ||
| flags.Var(&options.filter, "filter", `Provide filter values (e.g., "until=24h")`) | ||
| flags.Var(&options.keepStorage, "keep-storage", "Amount of disk space to keep for cache") | ||
| flags.Var(&options.minStorage, "min-storage", "Minimum amount of disk space to keep for cache") |
Member
There was a problem hiding this comment.
This part needs updates based on moby/buildkit#5359
Not a blocker but need to make sure we don't ship this version.
| func digestSliceFromPB(elems []string) []digest.Digest { | ||
| clone := make([]digest.Digest, len(elems)) | ||
| for i, e := range elems { | ||
| clone[i] = digest.Digest(e) |
Member
There was a problem hiding this comment.
Ideally all digest casts(when data comes from API) go through digest.Parse. There was a similar follow-up to buildkit PR.
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Oct 3, 2024
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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes gogo/protobuf from buildx and updates to a version of
moby/buildkit where gogo is removed.
This also changes how the proto files are generated. This is because
newer versions of protobuf are more strict about name conflicts. If two
files have the same name (even if they are relative paths) and are used
in different protoc commands, they'll conflict in the registry.
Since protobuf file generation doesn't work very well with
paths=source_relative, this removes thego:generateexpression andjust relies on the dockerfile to perform the generation.