-
Notifications
You must be signed in to change notification settings - Fork 820
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
Migrate from github.com/golang/protobuf to google.golang.org/protobuf #2786
Conversation
Build Failed 😱 Build Id: e9c4b831-7b07-465a-b0c7-82dc4d737da8 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 6eae69bd-7614-4e28-bb24-71015dede517 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 6ba80696-4dcf-4258-843a-ebb165662e78 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 9cf3c289-3da5-4ca7-8c27-b41426d570cc To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 06dac023-42a8-4d8d-b581-628d7e68a467 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: d07c8b4b-a4e7-456e-85cb-e8c2ce5d5870 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 309ff66f-d18f-4fb9-90f8-1fe31d5098b5 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 5b57e3df-70a9-4f2d-ad3c-4597eaf92b56 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
Woah. That's a tonne of work. Well done!
I threw some questions in there, but it's all minor stuff.
I'd like @roberthbailey to also take a look, since it's a big change, make sure I didn't miss anything important.
@@ -30,7 +30,7 @@ RUN git clone --recurse-submodules -b $GRPC_RELEASE_TAG --depth 1 --shallow-subm | |||
cd /var/local/git/grpc && \ | |||
mkdir -p cmake/build && \ | |||
cd cmake/build && \ | |||
cmake -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF ../.. && \ | |||
cmake -DCMAKE_BUILD_TYPE=Release -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF ../.. && \ |
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.
Out of curiosity, what does this do?
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.
This change triggered the agones-sdk-base image build with latest grpc-version.
Without this, the CI used cache of old image and failed to compile grpc codes.
In file included from /go/src/agones.dev/agones/sdks/cpp/include/agones/sdk.h:19,
from /go/src/agones.dev/agones/sdks/cpp/src/agones/sdk.cc:15:
/go/src/agones.dev/agones/sdks/cpp/include/agones/sdk.grpc.pb.h:548:17: error: 'grpc::MessageAllocator' has not been declared
548 | ::grpc::MessageAllocator< ::agones::dev::sdk::Empty, ::agones::dev::sdk::Empty>* allocator) {
| ^~~~~~~~~~~~~~~~
/go/src/agones.dev/agones/sdks/cpp/include/agones/sdk.grpc.pb.h:548:33: error: expected ',' or '...' before '<' token
548 | ::grpc::MessageAllocator< ::agones::dev::sdk::Empty, ::agones::dev::sdk::Empty>* allocator) {
| ^
/go/src/agones.dev/agones/sdks/cpp/include/agones/sdk.grpc.pb.h:575:17: error: 'grpc::MessageAllocator' has not been declared
575 | ::grpc::MessageAllocator< ::agones::dev::sdk::Empty, ::agones::dev::sdk::Empty>* allocator) {
| ^~~~~~~~~~~~~~~~
/go/src/agones.dev/agones/sdks/cpp/include/agones/sdk.grpc.pb.h:575:33: error: expected ',' or '...' before '<' token
575 | ::grpc::MessageAllocator< ::agones::dev::sdk::Empty, ::agones::dev::sdk::Empty>* allocator) {
| ^
...
-DCMAKE_BUILD_TYPE=Release
is needed for install grpc and its dependency, I think.
I referred to the official sample.
-- Could NOT find absl (missing: absl_DIR)
-- Configuring done
CMake Error at CMakeLists.txt:130 (add_library):
Target "agones" links to target "absl::flat_hash_map" but the target was
not found. Perhaps a find_package() call is missing for an IMPORTED
target, or an ALIAS target is missing?
...
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.
Oh got it!
You could have also updated
Base images for SDKs Bump: 2
To
Base images for SDKs Bump: 3
Just to update the file, which resets the build, but no reason to fix I don't think.
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.
Oh I didn't know # Base images for SDKs Bump: XX
!!!
After another review, if I should revert -DCMAKE_BUILD_TYPE=Release
, please let me know.
My apologies for my late reply, I was on a trip last week and had work for that period. |
All good! Thanks for doing all this work! |
Build Succeeded 👏 Build Id: 15337cbd-7e79-4a29-abab-7eadec7a404d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 0ad046f8-0024-4ddd-84cd-8bab2efb30dd The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
This looks good to me! No issues on my end.
@roberthbailey did you have a chance to look over this, since it's so big?
@zmerlynn would also love your eyes as well. I figure more reviews the better.
@@ -288,7 +286,7 @@ func runMux(h *serviceHandler, httpPort int) { | |||
grpcServer := grpc.NewServer(h.getMuxServerOptions()...) | |||
pb.RegisterAllocationServiceServer(grpcServer, h) | |||
|
|||
mux := gw_runtime.NewServeMux() | |||
mux := runtime.NewServerMux() |
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.
Noice.
@@ -30,7 +30,7 @@ RUN git clone --recurse-submodules -b $GRPC_RELEASE_TAG --depth 1 --shallow-subm | |||
cd /var/local/git/grpc && \ | |||
mkdir -p cmake/build && \ | |||
cd cmake/build && \ | |||
cmake -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF ../.. && \ | |||
cmake -DCMAKE_BUILD_TYPE=Release -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF ../.. && \ |
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.
Oh got it!
You could have also updated
Base images for SDKs Bump: 2
To
Base images for SDKs Bump: 3
Just to update the file, which resets the build, but no reason to fix I don't think.
This LGTM, but maybe we wait until RC is cut and then merge? That'll give it the whole next release to shake out. |
+1. I like this idea, but merge after 1.28 release is stable and out. So it has all of the 1.29 dev time to bake. |
Build Succeeded 👏 Build Id: e708a7e3-5098-478e-b61a-63a2919c1be9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Any objections to labelling this |
Build Succeeded 👏 Build Id: d460a383-bfa2-47d7-bfbc-1bd80ed2be10 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Hmm, if the protobufs are not wire compatible I would be worried. Do we have skew testing of any sort? Otherwise this feels wholly internal and I'm not sure we need to be super careful? |
I mean it should be fine, but like when uupgrading gRPC versions, I'm always like "definitely make sure to test release in dev, not in prod" 😄 |
This also includes a major version update of grpc-gateway. We've backward comparability tested as much as possible, but I figure good to give people the heads up. |
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: govargo, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It looks like this was merged to my repository(govargo/agones)'s main branch. https://github.com/govargo/agones/commits/migrate-protobuf Could you check this again? Thanks in advance. |
@govargo I believe that was @roberthbailey catching the branch up to |
Thank you all! |
…googleforgames#2786) Co-authored-by: Robert Bailey <[email protected]>
What type of PR is this?
/kind breaking
/kind cleanup
/kind feature
What this PR does / Why we need it:
I found that the
github.com/golang/protobuf
will be deprecated in the future when I executedmake gen-all-sdk-grpc
.This PR has many changes for migration to
google.golang.org/protobuf
and migration to grpc-gateway v2.Added
proto/grpc-gateway
allocation_grpc.pb.go
,alpha_grpc.pb.go
,beta_grpc.pb.go
,sdk_grpc.pb.go
,protoc-gen-openapiv2/options/annotations.pb.h
,protoc-gen-openapiv2/options/openapiv2.pb.h
,protoc-gen-openapiv2/annotations.pb.cc
,protoc-gen-openapiv2/openapiv2.pb.cc
)grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger
to proto files in order to define swagger.json definitionsprotoc-gen-openapiv2
protoc-gen-openapiv2/options
file's installation tosdks/cpp/CMakeLists.txt
sdks/cpp/sources.cmake
proto/grpc-gateway
tosdks/rust/build.rs
make gen-allocation-grpc
Changed
github.com/golang/protobuf
togoogle.golang.org/protobuf
(v1.28)github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
v1 togithub.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway
(v2.12.0)protoc
command options (e.g.--go_out=plugins=grpc
is not supported. use--go-grpc_opt
)github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2
instead ofprotoc-gen-swagger
google.golang.org/grpc/cmd/protoc-gen-go-grpc
v1.2google.golang.org/grpc
to v1.15.1grpc_release_tag
to v1.15.1 for sdk-base-imagegrpc_release_tag
to v1.15.1 forsdks/cpp/cmake/prerequisites.cmake
gen.sh
which has difference between each languagegrpc-tools
to 1.11.3 for nodejs sdk@grpc/grpc-js
to 1.7.3 andgoogle-protobuf
to 3.21.2 for nodejs sdkGoogle.Protobuf
to 3.21.9,Grpc
Grpc.Core
to 2.46.5,Grpc.Tools
to 2.50.0 in csharp sdksdks/rust/proto
which is copied for rust sdksdks/swagger/sdk.swagger.json
, which is changed byprotoc_gen_openapiv2
(x-stream-definitions is not already supported)test/sdk/restapi/http-api-test.go.nolint
code because return code was changedtest/sdk/websocket-watch/ws-watch-test.go
code because the test was not passed with the old codetools.go
dependenciesalpha.pb.go
,alpha.pb.gw.go
,beta.pb.go
,beta.pb.gw.go
,sdk.pb.go
,sdk.pb.gw.go
,sdk.grpc.pb.h
,sdk.pb.h
,google/api/annotations.pb.h
,google/api/http.pb.h
,sdk.grpc.pb.cc
,sdk.pb.cc
,google/annotations.pb.cc
,google/http.pb.cc
,Alpha.cs
,AlphaGrpc.cs
,Sdk.cs
,SdkGrpc.cs
,alpha_grpc_pb.js
,alpha_pb.js
,sdk_grpc_pb.js
,sdk_pb.js
)Deleted
github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
Workaround
sdks/swagger/sdk.swagger.json
because protoc-gen-openapiv2 doesn't work well in Stream and doesn't generate nessesary definitions. It seems that the bug occurs if both stream definition and--openapiv2_opt=disable_default_errors=true
exist.build/build-allocation-images/go/gen.sh
because protoc-gen-openapiv2 doesn't work well in description(even ifgrpc.gateway.protoc_gen_openapiv2.options.openapiv2_field
is added)Which issue(s) this PR fixes:
Closes #2462
Special notes for your reviewer:
After #2757, I don't add the swagger client go codes to this PR for testing backward compatibility.
I'll update swagger client go codes after this PR is merged.
This is large PR. I am very careful for this change, but if you have any suggestions, please let me know.
Thank you.