-
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
Updates to the C++ SDK along with the simple example that exercises it. #934
Conversation
c89eb19
to
5e7c02f
Compare
Build Failed 😱 Build Id: 90149902-db47-4d2d-b32d-921a0af8479d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: ad053951-a78b-435e-b73a-f82b7c656c53 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 8a753719-6659-41f4-a950-8c1451073da3 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:
|
6705608
to
287f70d
Compare
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.
Thanks for the updates.
Build Failed 😱 Build Id: f5f8a44c-b985-402f-a535-0e1483b41a88 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 301075d8-e263-4888-9f57-d15257250a6a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 0910b584-4dc4-4d2b-a4bb-d80636422d16 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: aa4d4d4b-f5e8-4988-a217-0918622d397f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
749fc1b
to
9187db9
Compare
Build Failed 😱 Build Id: 5f9f7585-188a-4f32-9eea-d73e9f45e2b1 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 0ad35a03-0620-489d-88af-82a835479654 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:
|
9187db9
to
7d214d3
Compare
Build Succeeded 👏 Build Id: 9c293644-93ab-464c-bc51-c8a9f83d70cd 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:
|
d9be549
to
5f7931b
Compare
Build Succeeded 👏 Build Id: 51026958-41e9-4102-bf91-36d94d145342 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: 98c5f88d-e480-4f80-940f-d1210a86e00c 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.
Overall, looks good. Thanks for the updates to the docs -- very helpful for me. A few small questions/comments.
@devjgm - this is ready for review. |
5f7931b
to
f02db01
Compare
Comments addressed; new version pushed. |
f02db01
to
dd8e8cc
Compare
Build Succeeded 👏 Build Id: 65d22200-e323-421b-ba1e-27828153505c 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 compile error in the version where the build just succeeded, which makes it pretty clear that our CI doesn't compile the example code. |
Build Succeeded 👏 Build Id: 753425f5-6d84-4296-80ce-fd601db9de76 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.
Code changes LGTM. Thanks.
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.
Lgtm, just some docs questions.
dd8e8cc
to
cc468a8
Compare
Build Succeeded 👏 Build Id: fe4927fa-41af-411b-9b52-774013a68d93 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:
|
cc468a8
to
fd35fd0
Compare
Build Succeeded 👏 Build Id: 09414444-8319-4c6a-8e52-eb13cd9a1a59 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:
|
128890d
to
a5ffbd0
Compare
Build Succeeded 👏 Build Id: 96a13ffb-1f0b-400b-97d6-ce22d80f7a8c 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: d43e6adb-5616-4750-a90c-9d9a045a40ac 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:
|
it. Create a tutorial to show how to build / run the simple C++ example.
a5ffbd0
to
d7a7b97
Compare
I've fixed the output buffering issues in the latest version. This should now be ready for another review pass. |
Build Succeeded 👏 Build Id: 981d47d3-bd16-42a5-8fb9-7eec92660f39 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 is great!
I've built |
Part of #703.
I ran
clang-format
onexamples/cpp-simple/server.cc
which accounts for many of the changes. The only code changes were modifyingstable::agones::...
toagones::...
.Both #855 and #904 broke the cpp-simple example code (which points to the need for us to have sdk conformance tests for C++):
In the former,
include/agones/sdk.grpc.pb.h
was no longer being installed with the sdk, which caused the sample code to no longer compile. I've restructured the cmake files a bit to make sure that it's now put into the expected place. /cc @dsazonoffIn the latter, the client context for the healthcheck calls was allocated on the stack in the constructor, but the healthchecks were continuously being called via the
Health
SDK function, which caused the program to abort abnormally due to a check failure. The code is now leaking a single client context, which we could clean up in the destructor if we wanted to be especially thorough (but in generally, since it has the same lifetime of the SDK and we expect the SDK to have the lifetime of the game server, this seemed not so bad compared to leaking one on each SDK call). /cc @devjgmNote that I haven't pushed the 0.6 version of the container yet, since I have some questions about whether the updates I made to the SDK are correct.