Migrate off of gogo/protobuf #4422

kzys · 2023-11-16T16:57:44Z

gogo/protobuf has been deprecated since 2022. This PR uses Google's official code generators instead of gogo's.

kzys · 2023-11-16T17:35:51Z

The fsutil change is in tonistiigi/fsutil#171.

tonistiigi · 2023-11-16T18:38:33Z

Is there a guarantee that this does not break old-new client-daemon combinations?

kzys · 2023-11-16T19:02:45Z

Yes. gogo/protobuf didn't modify the wire protocol. Their changes are primarily around code generators to make it more Go-idiomatic and efficient. I did a similar refactorting for containerd (see containerd/containerd#6564), and it has been released as containerd 1.7.0. We haven't heard any complaints regarding the compatibility.

thaJeztah · 2023-11-16T23:18:54Z

Seeing some test failures; are those expected?

=== FAIL: solver/llbsolver TestRecomputeDigests (0.00s)
    vertex_test.go:54: 
        	Error Trace:	D:/a/buildkit/buildkit/solver/llbsolver/vertex_test.go:54
        	Error:      	Not equal: 
        	            	expected: digest.Digest("sha256:06c243a9083f537e33d82b0af0379737187f7fbba73f852fe968b592c36bd8b5")
        	            	actual  : string("sha256:06c243a9083f537e33d82b0af0379737187f7fbba73f852fe968b592c36bd8b5")

kzys · 2023-11-17T04:07:34Z

Seeing some test failures; are those expected?

No. It should be fixed in the last revision, but the PR is still work-in-progress...

kzys · 2023-11-21T03:46:01Z

By fixing all golangci-lint errors, I ended up getting failed to load cache key: docker.io/library/busybox:latest@sha256:b28d6f3d483bfa5b30562287e1197fca960ca5dd8211d8cbf1536eb937e21af8: not found which I couldn't figure out why. So I'm re-doing this PR again with much smaller commits.

tonistiigi · 2023-11-29T02:00:20Z

api/types/worker.proto


 message WorkerRecord {
 	string ID = 1;
 	map<string, string> Labels = 2;
-	repeated pb.Platform platforms = 3 [(gogoproto.nullable) = false];
+	repeated pb.Platform platforms = 3;


Could we get some backward compatibility proving unit tests for the structs where the proto definition changed? Tests that use binary assets generated from previous version.

Let me check. I could write a few, but probably not for all of them. I feel that it is more like testing protos, not Buildkit.

It may make sense to do that in master first since this PR changes protos :)

Opened #4570.

tonistiigi · 2023-11-29T02:05:12Z

util/proto.go

@@ -0,0 +1,79 @@
+package util


This shouldn't be generic util pkg. Maybe util/cast ?

Sounds good.

Addressed by fc4c2fe

tonistiigi · 2023-11-29T02:06:24Z

frontend/gateway/grpcclient/client.go

@@ -51,11 +50,11 @@ func New(ctx context.Context, opts map[string]string, session, product string, c
 	}

 	if resp.FrontendAPICaps == nil {
-		resp.FrontendAPICaps = defaultCaps()
+		resp.FrontendAPICaps = util.PointerSlice(defaultCaps())


If it doesn't change anything else we should consider changing definition of defaultCaps() etc in these cases instead of converting.

Yeah. I was doing these conversions to change stuff incrementally. Let me check whether I can move that to the original functions instead of converting.

This PointerSlice and some other type-casting functions are gone at fc4c2fe.

sipsma · 2024-01-02T16:23:46Z

Are there any updates on the progress here? The only reason I care is that #4455 is accidentally dependent on it (see the PR description for why).

I totally get this PR may take some more time+effort to finish up, but if it's going to take a significantly longer time then it may be worth considering whether we should revert the changes in fsutil and have this PR depend on a fork of fsutil until it's ready to merge. Otherwise we'll be stuck in this state of not being able to update fsutil for a while.

cc @kzys (hi! long time no see 😄) @tonistiigi

kzys · 2024-01-20T05:20:47Z

Hi again @sipsma!

Sorry, I was blocking the review. Let me update the PR shortly.

Is it possible to get some benchmarks to understand how big issue this is and if we can stick with reflection or not.

@tonistiigi - Do you have benchmarks for Buildkit itself?

…obuf Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

gogo/protobuf has been deprecated since 2022. This change uses Google's official code generators instead of gogo's. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

With gogo, all protobuf-generated structs had Size() methods, and Size field was renamed to Size_ to avoid conflicts. With google.golang.org/protobuf, the structs simply have Size fields. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

In google.golang.org/protobuf, an empty message and nil are interchangeable. golang/protobuf#965 Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

google.golang.org/protobuf made all proto structs with a mutex to prevent copies. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

tonistiigi · 2024-04-02T17:09:50Z

Do you have benchmarks for Buildkit itself?

I think for this we would do micro-benchmarks for marshaling, unmarshal. If we start running containers then this would skew the timing too much. In fsutil there was also a benchmark script that maybe can be used. Maybe LLB marshal and load is a nice candidate.

The biggest possible performance issue would be with the session endpoint and the context and build result transfer. This is where 100s of MB can potentially be transferred and encoded/decoded. If session uses gRPC dialer then it also uses protobuf for raw data framing. I believe previously some of these decoding could avoid allocating more memory because the Unmarshal code was reusing the buffer by resetting it with [:0]. For these big data transfers, we should avoid new memory allocations that grow with amount of transferred data.

profnandaa · 2024-04-09T06:55:48Z

Just wondering, coz of the movements in the codebase, we are bound to have a lot of merge conflict as we go. Is possible to split this into some self-contained chunks like perhaps sub-packages and chain the PRs?

kzys force-pushed the bye-gogo branch 3 times, most recently from b518a14 to f81a7cc Compare November 16, 2023 17:33

kzys force-pushed the bye-gogo branch 5 times, most recently from c5ad2bd to 2979fbf Compare November 16, 2023 18:26

kzys force-pushed the bye-gogo branch 5 times, most recently from 2ba5d69 to f4aa840 Compare November 16, 2023 22:38

kzys force-pushed the bye-gogo branch from f4aa840 to 1b279ea Compare November 17, 2023 04:06

kzys force-pushed the bye-gogo branch 5 times, most recently from 0dead12 to d50b3b1 Compare November 21, 2023 03:40

kzys force-pushed the bye-gogo branch 5 times, most recently from ff8e07b to d7e32bb Compare November 21, 2023 18:18

tonistiigi reviewed Nov 29, 2023

View reviewed changes

sipsma mentioned this pull request Dec 2, 2023

add support for AlwaysReplaceExistingDestPaths in llb copy #4455

Merged

kzys added 3 commits January 19, 2024 21:24

Upgrade fsutil to use google.golang.org/protobuf instead of gogo/prot…

8368baf

…obuf Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Don't use gogo/protobuf's code generator

5b7ba13

gogo/protobuf has been deprecated since 2022. This change uses Google's official code generators instead of gogo's. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Rename Size_ to Size

6c10439

With gogo, all protobuf-generated structs had Size() methods, and Size field was renamed to Size_ to avoid conflicts. With google.golang.org/protobuf, the structs simply have Size fields. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys force-pushed the bye-gogo branch from 1aad506 to 68d8112 Compare January 20, 2024 05:39

Fix build errors

68652aa

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys force-pushed the bye-gogo branch from 68d8112 to 8a2b2ff Compare January 20, 2024 05:49

kzys added 4 commits January 19, 2024 22:07

Fix tests

e2217bb

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Treat an empty message as nil

e8bb0ce

In google.golang.org/protobuf, an empty message and nil are interchangeable. golang/protobuf#965 Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Remove gogo

3cc72fd

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

Fix lint issues

e801e0e

google.golang.org/protobuf made all proto structs with a mutex to prevent copies. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys force-pushed the bye-gogo branch from 8a2b2ff to e801e0e Compare January 20, 2024 06:07

Move type-casting functions from util to util/cast

fc4c2fe

Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys added a commit to kzys/buildkit that referenced this pull request Jan 20, 2024

Keep serialized protobuf binaries for testing

0eea591

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys mentioned this pull request Jan 20, 2024

Keep serialized protobuf binaries for testing #4570

Open

crazy-max mentioned this pull request Jan 29, 2024

Revert "Migrate off of gogo/protobuf" tonistiigi/fsutil#176

Merged

kzys added a commit to kzys/buildkit that referenced this pull request Feb 1, 2024

Keep serialized protobuf binaries for testing

ac3a02c

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys added a commit to kzys/buildkit that referenced this pull request Feb 1, 2024

Keep serialized protobuf binaries for testing

d4e6c66

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys added a commit to kzys/buildkit that referenced this pull request Feb 1, 2024

Keep serialized protobuf binaries for testing

cad2c8e

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys added a commit to kzys/buildkit that referenced this pull request Feb 1, 2024

Keep serialized protobuf binaries for testing

4918e71

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

kzys added a commit to kzys/buildkit that referenced this pull request Feb 1, 2024

Keep serialized protobuf binaries for testing

8a4be6e

These serialized binaries can be used to test changes like moby#4422. Signed-off-by: Kazuyoshi Kato <kato.kazuyoshi@gmail.com>

thompson-shaun added the status/needs-attention label May 16, 2024

thompson-shaun added this to the v0.next milestone May 23, 2024

thompson-shaun added dependencies Pull requests that update a dependency file and removed status/needs-attention labels Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate off of gogo/protobuf #4422

Migrate off of gogo/protobuf #4422

kzys commented Nov 16, 2023 •

edited

Loading

kzys commented Nov 16, 2023

tonistiigi commented Nov 16, 2023

kzys commented Nov 16, 2023

thaJeztah commented Nov 16, 2023

kzys commented Nov 17, 2023

kzys commented Nov 21, 2023 •

edited

Loading

tonistiigi Nov 29, 2023

kzys Jan 20, 2024

kzys Jan 20, 2024

kzys Jan 20, 2024

tonistiigi Nov 29, 2023

kzys Jan 20, 2024

kzys Jan 20, 2024

tonistiigi Nov 29, 2023

kzys Jan 20, 2024 •

edited

Loading

kzys Jan 20, 2024

sipsma commented Jan 2, 2024

kzys commented Jan 20, 2024

tonistiigi commented Apr 2, 2024

profnandaa commented Apr 9, 2024

Migrate off of gogo/protobuf #4422

Are you sure you want to change the base?

Migrate off of gogo/protobuf #4422

Conversation

kzys commented Nov 16, 2023 • edited Loading

kzys commented Nov 16, 2023

tonistiigi commented Nov 16, 2023

kzys commented Nov 16, 2023

thaJeztah commented Nov 16, 2023

kzys commented Nov 17, 2023

kzys commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzys Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipsma commented Jan 2, 2024

kzys commented Jan 20, 2024

tonistiigi commented Apr 2, 2024

profnandaa commented Apr 9, 2024

kzys commented Nov 16, 2023 •

edited

Loading

kzys commented Nov 21, 2023 •

edited

Loading

kzys Jan 20, 2024 •

edited

Loading