Cpp sdk cmake#464
Merged
markmandel merged 1 commit intoagones-dev:masterfrom Mar 25, 2019
dsazonoff:cpp_sdk_cmake
Merged
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Collaborator
|
Build Succeeded 👏 Build Id: c83db526-3df2-49cb-a1da-60658a1c4b04 The following development artifacts have been built, and will exist for the next 30 days:
To install this version:
|
Collaborator
|
Build Succeeded 👏 Build Id: b6e5fc55-7040-4cdc-a896-29fd326bf039 The following development artifacts have been built, and will exist for the next 30 days:
To install this version:
|
Contributor
Author
|
CLA signed, please review. |
|
CLAs look good, thanks! |
markmandel
reviewed
Dec 30, 2018
Collaborator
markmandel
left a comment
markmandel
left a comment
There was a problem hiding this comment.
This looks like a good start, but I have some questions.
Also looks like some functionality and good documentation from the previous PR got dropped, that I think would be valuable.
Question: Should this PR also be integrated into cloudbuild.yaml ? Or do we want to do CI in a follow up PR?
@alexandrem - your C++ is better than mine. Did you want to do a pass?
sdks/cpp/README.md
Outdated
| ## Building From Source | ||
| To build Agones C++ SDK from sources you need to set up your development environment and install following tools: | ||
| - CMake (https://cmake.org/download/) | ||
| - OpenSSL (https://github.com/openssl/openssl) |
Collaborator
There was a problem hiding this comment.
Why does the user need to install OpenSSL? We actually don't need SSL termination in our SDK, so if we can remove this, that would be ideal. In the previous incarnation, we just built out grpc++_unsecure
There was a problem hiding this comment.
If I recall correctly, grpc doesn't offer much in terms of customization and it isn't technically possible to compile it without openssl.
Collaborator
There was a problem hiding this comment.
If I'm reading the previous PR correctly, it looks you can compile it without SSL. But maybe I'm reading it wrong? 🤷♂️
There was a problem hiding this comment.
Which previous PR?
But if you look at grpc cmake https://github.com/grpc/grpc/blob/master/CMakeLists.txt#L141
and https://github.com/grpc/grpc/blob/master/cmake/ssl.cmake
there isn't really any way to turn it off.
GRPC cmake (I believe) is generated, and really hard to follow.
It is probably possible to build specifically the grpc_unsecure target, and set gRPC_SSL_PROVIDER to module.
We really would need an automated cmake build for this repo though. A lot of surprises will pop up since its currently untested,
Collaborator
There was a problem hiding this comment.
This previous PR: #421
It is probably possible to build specifically the grpc_unsecure target, and set gRPC_SSL_PROVIDER to module
Can we do that :) ?
We really would need an automated cmake build for this repo though. A lot of surprises will pop up since its currently untested,
Yes - agreed. We should integrate this PR into the cloudbuild system. Next steps should be to make some tests, and also setup CI with windows (AppVeyor?) and OSX (Travis I think does this?) as well.
There was a problem hiding this comment.
When you add a git external project in cmake, it automatically updates submodules. In the case of the previous PR, this was the case. One of GRPC submodule happens to be boringSSL :)
This is probably what will end up happening in this PR when no GRPC path is specified.
sdks/cpp/README.md
Outdated
| - CMake (https://cmake.org/download/) | ||
| - OpenSSL (https://github.com/openssl/openssl) | ||
| - Git (https://git-scm.com/downloads) | ||
| - gRPC (https://github.com/grpc/grpc) |
Collaborator
There was a problem hiding this comment.
What does installing gRPC mean here?
The cmake installation should be as turnkey as possible. If they user has to manually install gRPC themselves, this defeats the purpose of the tool we're trying to make here. The user really shouldn't care what the communication protocol is.
The previous incarnation did this really well:
https://github.com/GoogleCloudPlatform/agones/pull/421/files#diff-8ffc59e90c0bca72cf7710844b68352dR28
Contributor
Author
There was a problem hiding this comment.
Cpp SDK depends on gRPC. Unfortunately, we can't simply install gRPC for C++, as mentioned here. So we need to do a manual build of gRPC which depends on OpenSSL and so on.
I think we should do next steps, to build SDK and avoid OpenSSL dependency:
- Download gRPC repo (if path to gRPC is not specified)
- Build only
grpc_unsecuretarget (gRPC cmake package scripts will not be generated) as separate build step - Use our own find package script that will have all references to include/lib directories of gRPC
- Build SDK
3rd step is necessary, so end user can link with SDK, gRPC and all other dependencies without hardcoding. We need to support both Debug/Release builds for IDE-s such XCode and MSVS.
@sylvainduchesne what do you think?
sdks/cpp/README.md
Outdated
| - [protobuf](https://developers.google.com/protocol-buffers/), v3.5.0 - [C++ compilation guide](https://github.com/google/protobuf/blob/master/src/README.md) | ||
| choco install git cmake activeperl golang yasm | ||
|
|
||
| It is possible to use your own build of gRPC. In this case you need to make your **gRPC** and **protobuf** installation visible to CMake, so `find_package` command can access all necessary cmake scripts. |
Collaborator
There was a problem hiding this comment.
I don't think we really want users to be doing this. They have no need to - and it's going to be a source of pain for us to maintain. We should dictate the gRPC installation version, and it's install process, and manage it for the user.
To make things simpler for everyone, I think we should remove all customisation options for being able to provide your own gRPC. Our build system should manage this for the user end to end.
There was a problem hiding this comment.
External dependencies can be kind of tricky. If a project already uses grpc (or other) you can't expect them to have 2 copies of the same library at different versions. For this reason, its generally better to allow for this customization.
There was a problem hiding this comment.
That's not to say that we cannot provide a version of grpc if one isn't specified however.
Collaborator
There was a problem hiding this comment.
Then we should definitely provide a default if one isn't provided 👍
I'm just very scared of trying to support someone running a random version of gRPC, and then expecting us to support it.
Contributor
Author
There was a problem hiding this comment.
Projects may use gRPC for their own needs and in common way it is not possible to mix different versions of gRPC in one binary.
It is possible to do it only with some restrictions: only if SDK was built as dynamic library with statically linked gRPC. It means that client will use its own instance of gRPC, and SDK will use its own.
But it is a danger way, because both instances of gRPC (application's and SDK's) may share same kernel objects (named mutex, shared memory etc.) which may cause unexpected problems. To know for sure it is necessary to know gRPC internals in depth.
My suggestion is to provide a choice to an end user how to link with gRPC. But we can show a warning if user tries to use unexpected version of gRPC to build SDK. Or even to disable the build unless user explicitly state that he wants to use different version of gRPC. In this case we can support only exact version of gRPC; in other cases user will be responsible for possible problems.
Collaborator
There was a problem hiding this comment.
My suggestion is to provide a choice to an end user how to link with gRPC. But we can show a warning if user tries to use unexpected version of gRPC to build SDK.
This sounds like the perfect option to me 👍
gRPC is generally SUPER backward compatible, but we should show a warning just in case.
sdks/cpp/README.md
Outdated
| If you want to build the libraries from Agones source, | ||
| the `make` target `build-sdk-cpp` will compile both static and dynamic libraries for Debian/Linux | ||
| for your usage, to be found in the `bin` directory inside this one. | ||
| To build gRPC you may also need to install next dependencies: |
Collaborator
There was a problem hiding this comment.
The previous PR's documentation covered this really nicely, and specifically went through each dependency:
https://github.com/GoogleCloudPlatform/agones/pull/421/files#diff-eeb2d8fcc3782771feb2798fb61b5979R135
Collaborator
|
Apparently, I cc'd the wrong person 😊 |
markmandel
added
the
feature-freeze-do-not-merge
|
I think the PR looks good, but I would like to see a proper cmake package so that agones could also be consumed as a cmake package. Would it be possible to add this to the PR? |
markmandel
removed
feature-freeze-do-not-merge
Contributor
Author
Yes, working on it now. |
markmandel
added
kind/feature
aLekSer
reviewed
Jan 15, 2019
Collaborator
|
Build Failed 😱 Build Id: a7e35af6-6fda-4b94-aaf8-41461e256255 Build Logs |
aLekSer
approved these changes
Jan 16, 2019
Collaborator
aLekSer
left a comment
aLekSer
left a comment
There was a problem hiding this comment.
Fine with years, which are according the policy
sdks/cpp/sdk_global.h
Outdated
| @@ -0,0 +1,24 @@ | |||
| // Copyright 2017 Google Inc. All Rights Reserved. | |||
Collaborator
There was a problem hiding this comment.
Same as above - year change
sdks/cpp/README.md
Outdated
| that platform, at this time you will need to compile from source. Windows and macOS libraries | ||
| for the C++ SDK for easier cross platform development are planned and will be provided in the near future. | ||
| ### Building | ||
| There are `cpp/build_scripts/CMakeLists.txt` file that can be used to build Agones C++ SDK with gRPC and other dependencies (pre-builded OpenSSL is still required). |
Collaborator
|
Build Failed 😱 Build Id: 43011ac3-f184-43c6-bf0e-4f9c4110a9a5 Build Logs |
Collaborator
|
Build Failed 😱 Build Id: d7152a7c-003e-47a7-8d53-e502cfba0384 Build Logs |
Collaborator
|
Build Failed 😱 Build Id: ab27a8a8-3c6c-4b01-90d3-7c5710279698 Build Logs |
Collaborator
|
Build Failed 😱 Build Id: ffd7b46e-7164-4d6e-8d2b-cb639133c6cf To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
markmandel
added
the
feature-freeze-do-not-merge
Collaborator
|
Build Failed 😱 Build Id: 4463fc17-19bd-4326-b626-c1ec3dbb2ef7 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Collaborator
|
Build Succeeded 👏 Build Id: f75cf9dc-78d1-4c3c-8c3c-78b9be36374d 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:
|
|
Just tested the latest code, looks good! Sorry took a while, I recently switched computers at work. |
Contributor
Author
|
@markmandel , I think we can merge? |
Collaborator
|
The only minor tweak I would make - on the documentation page, rather than replace the docs, keep the old documentation, and wrap it in a Details on how to do this here: https://agones.dev/site/docs/contribute/ Other than that, LGTM! |
Collaborator
|
Build Succeeded 👏 Build Id: dacf3cdc-30cb-4e08-a70f-f5913f2f3ac6 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:
|
Collaborator
|
Build Succeeded 👏 Build Id: feeaea80-4ea5-4cd2-9b0e-236806fadc68 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:
|
Collaborator
|
Build Failed 😱 Build Id: cb7bbec7-ea5e-406d-8f62-13fb17cf8367 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Collaborator
|
Build Succeeded 👏 Build Id: 29f86512-c87f-4541-8b9f-e690bb2534c4 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:
|
Collaborator
|
Build Succeeded 👏 Build Id: 3b90b300-faad-4a41-90ee-8a9e285b51d6 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:
|
Contributor
Author
@markmandel done |
markmandel
reviewed
Mar 11, 2019
markmandel
reviewed
Mar 11, 2019
| CHART_DIR=install/.helm-$(RELEASE_VERSION)/agones $(MAKE) push-chart | ||
|
|
||
| #build all the sdks | ||
| build-sdks: build-sdk-cpp |
Collaborator
There was a problem hiding this comment.
@dsazonoff should this have been removed? Should we build and/or test this SDK on each run, in case something fails?
Contributor
Author
There was a problem hiding this comment.
I just removed c++ related part. Probably yes, we should remove this step.
Collaborator
There was a problem hiding this comment.
I guess my point is - shouldn't building the SDK be part of the tests, or ensure there isn't some error in it?
Collaborator
|
Build Failed 😱 Build Id: ea50d11b-9d11-443b-bf4c-fc43a14679ac To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Collaborator
|
Build Succeeded 👏 Build Id: 52656650-8377-46ab-bae5-eb469467fc0d 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:
|
Collaborator
|
Build Succeeded 👏 Build Id: 85245477-d69c-41fc-ba9c-9216fe5bcb0b 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:
|
Collaborator
|
@dsazonoff this looks great! Please squash to a single commit, and resolve the above conflicts, and I'll approve and merge (just in time for RC on Tuesday!) |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
Collaborator
|
Build Failed 😱 Build Id: 94d0e9a0-4d17-4f83-9be6-6a3dce690091 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Collaborator
|
Build Failed 😱 Build Id: 2651a48e-f218-422d-b365-2de28b319a17 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Contributor
Author
Done, merged. I'm not sure, why build is failing. Locally everything is built. |
Collaborator
|
Build Succeeded 👏 Build Id: 58c828d1-584a-48d3-b99c-e2793d76940f 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:
|
Collaborator
|
Build Succeeded 👏 Build Id: fff88248-4d88-464e-80b4-ff960a6be4c1 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:
|
Collaborator
|
Build Succeeded 👏 Build Id: 436fa4ff-5b24-4e33-bd6f-26b0a2ec70ae 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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changeset provides CMake scripts for building C++ SDK that works across Windows, Linux and macOS.
Old Makefile will be removed after migrating examples/cpp-simple to CMake.
Issue: #134