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.

Sign up for GitHub

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

Jump to bottom

Bugs and Improvements for CPP SDK and Example #3318

Merged
merged 4 commits into from
Aug 11, 2023
Merged

Bugs and Improvements for CPP SDK and Example #3318

merged 4 commits into from
Aug 11, 2023

Conversation

markmandel
Copy link
Member

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:

This fixes a bug in the CPP SDK, that was highlighted in the CPP example, but didn't end show up in the conformance tests, as well as several quality of life improvements.

  • Bring all the gRPC references to our current gRPC version in the project.
  • Add -j$(nproc) for all build targets to utlise more cores when compiling. Shaved some time off compilation.
  • Make the setup and compilation of the example align with the unit tests (where it makes sense) - so same base image, etc.
  • Explicitly compile the Abseil dependency (this was the critical issue!)

Which issue(s) this PR fixes:

Work on #3281

Special notes for your reviewer:

Not quite sure if there is a good way to force this kind of issue to show up in the conformance tests, given that we rely on gRPC to be installed on the base image all SDK images are derived from -- but at least this gets us over the current hurdle.

@markmandel
Bugs and Improvements for CPP SDK and Example
1452af3 
This fixes a bug in the CPP SDK, that was highlighted in the CPP
example, but didn't end show up in the conformance tests, as well as
several quality of life improvements.

* Bring all the gRPC references to our current gRPC version in the
  project.
* Add `-j$(nproc)` for all `build` targets to utlise more cores when
compiling. Shaved some time off compilation.
* Make the setup and compilation of the example align with the unit
tests (where it makes sense) - so same base image, etc.
* Explicitly compile the Abseil dependency (this was the critical
issue!)

Not quite sure if there is a good way to force this kind of issue to
show up in the conformance tests, given that we rely on gRPC to be
installed on the base image all SDK images are derived from -- but at
least this gets us over the current hurdle.

Work on googleforgames#3281
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc and removed approved labels Aug 9, 2023
@github-actions github-actions bot added the kind/bug These are bugs. label Aug 9, 2023
@google-oss-prow google-oss-prow bot added the lgtm label Aug 9, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gongmax, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel
Copy link
Member Author

@Kalaiselvi84 once this is merged, this should unblock you on completing #3281 👍🏻

@Kalaiselvi84 Kalaiselvi84 mentioned this pull request Aug 10, 2023
Closed
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b77db227-c367-4165-af83-9d15fcd0c33e

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 51f9a6fa-8518-4050-8b1b-e141f17be0a2

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

@google-oss-prow google-oss-prow bot removed the lgtm label Aug 10, 2023
@google-oss-prow
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 87ba9914-bdef-4dd2-b6a8-51c913ace3d2

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

@markmandel
Copy link
Member Author 

CMake Error at CMakeLists.txt:32 (_message):
  Some (but not all) targets in this export set were already defined.

  Targets Defined:
  gRPC::re2;gRPC::address_sorting;gRPC::gpr;gRPC::grpc;gRPC::grpc_unsecure;gRPC::grpc++;gRPC::grpc++_alts;gRPC::grpc++_error_details;gRPC::grpc++_reflection;gRPC::grpc++_unsecure;gRPC::grpc_plugin_support;gRPC::grpcpp_channelz;gRPC::upb


  Targets not yet defined: gRPC::grpc_authorization_provider

Call Stack (most recent call first):
  .build/.install/gRPC/lib/cmake/grpc/gRPCTargets.cmake:37 (message)
  .build/.install/gRPC/lib/cmake/grpc/gRPCConfig.cmake:23 (include)
  CMakeLists.txt:77 (find_package)


-- 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

This is passing locally, so I'm very confused.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7ef404bf-4f4b-49e8-8433-af4621bb353f

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/3318/head:pr_3318 && git checkout pr_3318
  • 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.34.0-dev-045f407-amd64

@markmandel markmandel enabled auto-merge (squash) August 10, 2023 23:53
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 1838b5e8-6748-48ea-a396-6265afd7854e

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/3318/head:pr_3318 && git checkout pr_3318
  • 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.34.0-dev-2aebc59-amd64

@markmandel markmandel merged commit b097ed0 into googleforgames:main Aug 11, 2023
3 checks passed
@markmandel markmandel deleted the draft/cpp-issue branch August 11, 2023 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gongmax gongmax gongmax approved these changes

@zmerlynn zmerlynn Awaiting requested review from zmerlynn

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

@aLekSer aLekSer Awaiting requested review from aLekSer

@EricFortin EricFortin Awaiting requested review from EricFortin

Assignees

@gongmax gongmax

Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markmandel @agones-bot @gongmax