Skip to content
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

Upgrade gRPC version #3472

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Oct 28, 2023

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3467

Special notes for your reviewer:

@github-actions github-actions bot added the kind/cleanup Refactoring code, fixing up documentation, etc label Oct 28, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b881305e-50cd-40b1-be3f-1a9f1dc4b869

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@Kalaiselvi84
Copy link
Contributor Author

@markmandel, Could you please guide me how to fix this error?

CMake Error at CMakeLists.txt:32 (_message):
  CMake Error: The source directory
  "/go/src/agones.dev/agones/sdks/cpp/.build/gRPC/src/third_party/protobuf/cmake"
  does not appear to contain CMakeLists.txt.

  Specify --help for usage, or press the help button on the CMake GUI.

Call Stack (most recent call first):
  cmake/prerequisites.cmake:93 (message)
  cmake/prerequisites.cmake:114 (execute_and_check)
  cmake/prerequisites.cmake:174 (invoke_cmake_build)
  CMakeLists.txt:73 (include)


-- Configuring incomplete, errors occurred!
See also "/go/src/agones.dev/agones/sdks/cpp/.build/CMakeFiles/CMakeOutput.log".
See also "/go/src/agones.dev/agones/sdks/cpp/.build/CMakeFiles/CMakeError.log".
make: *** [Makefile:29: build] Error 1
make[1]: *** [includes/sdk.mk:88: run-sdk-command] Error 2
make: *** [includes/sdk.mk:72: run-sdk-command-cpp] Error 2

@markmandel
Copy link
Member

Will pull this down and take a look 👍🏻

@markmandel
Copy link
Member

markmandel commented Oct 30, 2023

So just digging into this - looks like you may have run make gen-sdk-grpc (which defaults to go) not make gen-all-sdk-grpc (which runs against all the SDKs) since only the Go code got updated (also I get extra updates when I run it locally). Easy thing to do as the commands are next to each other -- we should probably turn off the default for gen-sdk-grpc.

if you run make gen-all-sdk-grpc you should see changes that look more like this:

❯ git shell
On branch pr/grpc-update
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   pkg/sdk/alpha/alpha.pb.go
        modified:   pkg/sdk/alpha/alpha_grpc.pb.go
        modified:   pkg/sdk/beta/beta.pb.go
        modified:   pkg/sdk/beta/beta_grpc.pb.go
        modified:   pkg/sdk/sdk.pb.go
        modified:   pkg/sdk/sdk_grpc.pb.go
        modified:   sdks/cpp/include/agones/sdk.pb.h
        modified:   sdks/cpp/include/google/api/annotations.pb.h
        modified:   sdks/cpp/include/google/api/http.pb.h
        modified:   sdks/cpp/include/protoc-gen-openapiv2/options/annotations.pb.h
        modified:   sdks/cpp/include/protoc-gen-openapiv2/options/openapiv2.pb.h
        modified:   sdks/cpp/src/agones/sdk.pb.cc
        modified:   sdks/cpp/src/google/annotations.pb.cc
        modified:   sdks/cpp/src/google/http.pb.cc
        modified:   sdks/cpp/src/protoc-gen-openapiv2/annotations.pb.cc
        modified:   sdks/cpp/src/protoc-gen-openapiv2/openapiv2.pb.cc

Not 100% sure it solves the problem yet, but just working step by step.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ea09cb33-2c58-4c7e-aa64-ffbcb7966580

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@Kalaiselvi84
Copy link
Contributor Author

Log for make gen-all-sdk-grpc - https://gist.github.com/Kalaiselvi84/68c31b262ca04b5b57275f049e438232

You're right; I may have executed make gen-sdk-grpc. 6 files were modified locally for the command make gen-all-sdk-grpc, but it seems more files were altered in yours.🤔

@markmandel
Copy link
Member

I got the same error anyway with all the files updated, but I think i have a fix.... lemme try it though.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 57d9f6d0-d92d-400b-84f6-dcc5aa86b583

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Member

markmandel commented Oct 31, 2023

Fairly sure I got it... I'll do my usual file a PR against your branch thing.

@markmandel
Copy link
Member

I cleaned my build and am running it again - so probably have confirmation tomorrow.

@markmandel
Copy link
Member

Kalaiselvi84#326 submitted to your branch!

The CPP tests passed for me 👍🏻

@Kalaiselvi84
Copy link
Contributor Author

I got the same error anyway with all the files updated, but I think i have a fix.... lemme try it though.

I am confused. What's the difference between my local setup and yours? Why aren't all the files being modified on my end as they are on yours?

@markmandel
Copy link
Member

I am confused. What's the difference between my local setup and yours? Why aren't all the files being modified on my end as they are on yours?

I figure it's because I had rebuilt all my Docker images, so it pulled down the latest protobuf (which is why the version updated). Also because I ran the make gen-allocation-grpc command.

@Kalaiselvi84
Copy link
Contributor Author

I figure it's because I had rebuilt all my Docker images, so it pulled down the latest protobuf (which is why the version updated). Also because I ran the make gen-allocation-grpc command.

When you mentioned rebuilding all the Docker images, did you mean only for the example images or did you rebuild everything? Also I guess you ran make build instead make build-sdks

@markmandel
Copy link
Member

markmandel commented Oct 31, 2023

When you mentioned rebuilding all the Docker images, did you mean only for the example images or did you rebuild everything?

I occasionally delete all docker images from my machines (docker system prune -a --volumes) just to keep them clean, and so I was rebuilding everything from scratch.

Either way - merge my PR to your PR when ready, and we'll see if it passes all of CI 👍🏻

@Kalaiselvi84
Copy link
Contributor Author

rebuilding everything from scratch.

Do we have any command for this?

Either way - merge my PR to your PR when ready, and we'll see if it passes all of CI 👍🏻

This is kind of no fair to always use your updates, would it be possible for you to create a separate PR for it? What do you think?

else share me your logs, I will try to replicate yours

@markmandel
Copy link
Member

Do we have any command for this?

I just do the command above occasionally when I do docker images and there are a lot of them there 😄

This is kind of no fair to always use your updates, would it be possible for you to create a separate PR for it? What do you think?

I don't mind either way - whatever works!

@google-oss-prow google-oss-prow bot added size/XXL and removed size/L labels Oct 31, 2023
@markmandel
Copy link
Member

Also just realised I was typing on my phone above and wrote make gen-allocation-grpc when I meant make gen-allocation-grpc 🤦🏻

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5594bbca-edaf-4c65-982f-576385f624d6

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@Kalaiselvi84
Copy link
Contributor Author

Also just realised I was typing on my phone above and wrote make gen-allocation-grpc when I meant make gen-allocation-grpc

This was also typing from your phone? you typed the same command here😂

@markmandel
Copy link
Member

Also just realised I was typing on my phone above and wrote make gen-allocation-grpc when I meant make gen-allocation-grpc

This was also typing from your phone? you typed the same command here😂

OMG I did it again. make gen-all-sdk-grpc I can do it!

@markmandel
Copy link
Member

markmandel commented Oct 31, 2023

As part of this PR can we update make test-gen-all-sdk-grpc to show what the differences are?

@Kalaiselvi84
Copy link
Contributor Author

As part of this PR can we update make test-gen-all-sdk-grpc to show what the differences are?

make test-gen-all-sdk-grpc is running, will update the status shortly

@markmandel
Copy link
Member

As part of this PR can we update make test-gen-all-sdk-grpc to show what the differences are?

make test-gen-all-sdk-grpc is running, will update the status shortly

Don't we need that update in this PR? We don't know what is being generated by Cloud Build and how it's different to what we have right now.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e48cf338-8a6e-4f29-8b8a-826a66b08a94

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Kalaiselvi84 and others added 6 commits October 31, 2023 11:50
* Fix for cmake grpc dependency
* Updated all gRPC SDK code.
* Include the gRPC version in the `build_sdk_base_version` version such
that it automatically gets rebuilt whenever the gRPC version changes
without manual intervention.

Also updated the changes in the allocation gRPC code as well.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 180da6c3-c541-4b24-8cc7-8ba3cc5967ba

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3472/head:pr_3472 && git checkout pr_3472
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.36.0-dev-8238032-amd64

@Kalaiselvi84
Copy link
Contributor Author

200% credit goes to @markmandel 😀

build_sdk_base_version = $(call sha,$(build_path)/build-sdk-images/tool/base/Dockerfile)
grpc_release_tag = v1.57.0

build_sdk_base_version = $(call sha,$(build_path)/build-sdk-images/tool/base/Dockerfile)_$(grpc_release_tag)
Copy link
Member

Choose a reason for hiding this comment

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

This now forces a rebuild when the grpc version is updated. Before this wasn't happening.

@google-oss-prow google-oss-prow bot added the lgtm label Oct 31, 2023
@markmandel markmandel merged commit 01fc87b into googleforgames:main Oct 31, 2023
3 checks passed
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gongmax, Kalaiselvi84, zmerlynn
Once this PR has been reviewed and has the lgtm label, please assign ericfortin for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Kalaiselvi84 Kalaiselvi84 deleted the grpc-update branch March 15, 2024 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Refactoring code, fixing up documentation, etc lgtm size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup: Synchronise gRPC versions across the repository
5 participants