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

Cpp prerequisities cmake #803

Merged
merged 1 commit into from
Jul 12, 2019
Merged

Cpp prerequisities cmake #803

merged 1 commit into from
Jul 12, 2019

Conversation

dsazonoff
Copy link
Contributor

No description provided.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 3a67c033-d5d7-4f24-a893-055204d24e8a

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-79da10d

@dsazonoff dsazonoff mentioned this pull request Jun 3, 2019
Closed
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 4002ea2a-cd28-467e-9190-a925d1b864d1

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 2453347c-1751-488f-bc2d-67d6782830e2

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-262b187

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0c92e084-9bd0-4f78-afc6-74fa9731570f

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 53680f0b-491e-46b9-bce5-d4339636d531

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-325999e

@dsazonoff dsazonoff marked this pull request as ready for review June 10, 2019 16:08
@dsazonoff
Copy link
Contributor Author

dsazonoff commented Jun 10, 2019 •
edited
Loading

Main change of this PR: there is a prerequisities.cmake script that simplify a build process for different cmake-based third-parties in a cross-platform way. Previous solution, where gRPC+protobuf has only partial build and manual installation is removed (discussion was in #464).
Option for building Agones as dynamic library is removed too (it is a topic to discuss). According to protobuf recomendation it is necessary to wrap all code so there will be no exported gRPC/Protobuf symbols. I want to restore this option in a next PR. It will completely remove gRPC+protobuf dependency for dynamic build.
@sylvainduchesne - could you help with code review, please?

@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/cleanup Refactoring code, fixing up documentation, etc labels Jun 10, 2019
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e256a4ae-cf94-47eb-ae22-7fa0605ad1f6

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-daa9104

@markmandel
Copy link
Member

Gentle bump to @sylvainduchesne - can you review please. I've no idea what any of this does 😄

@sylvainduchesne
Copy link

I didn't get a chance to try it, but nothing jumps out as being a problem in this PR

@markmandel markmandel added this to the 0.12.0 milestone Jun 18, 2019
@markmandel markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jun 18, 2019
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e1b929a3-aae2-4adc-97e3-3614534a3506

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-fcb9e75

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8611204b-fe39-4c32-9155-d9bb068aa6b8

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-9ca8a89

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d23ab077-6670-461f-a1fe-2f525c31ffb3

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.11.0-b532943

@sylvainduchesne
Copy link

sylvainduchesne commented Jun 19, 2019 •
edited
Loading

I tried the branch and it works, although, wow it takes about 30 minutes to build (well on Windows anyways).

One thing I did note however, the install target seems to only install agones and not any of the dependencies we built. Since we will need these to build and link, it would probably be helpful to call install on all targets.

@dsazonoff
Copy link
Contributor Author

dsazonoff commented Jun 20, 2019 •
edited
Loading

I'm not sure, should we install whole gRPC or not? There are no options for partial installation of gRPC.
But there is an AGONES_THIRDPARTY_INSTALL_PATH variable, it is possible to specify installation path for third party (default is ${CMAKE_BINARY_DIR}/third_party). Probably I should change it to CMAKE_INSTALL_PREFIX value. But original idea was not to mix Agones SDK and third party installations.
Usual case for development is when you have a project (game server) and you have built and installed all prerequisites, so path for Agones, gRPC, Protobuf etc. should be specified by developer.
Note about build time. For Visual Studio it is necessary to build debug and release configurations - that's doubles a build time.

it would probably be helpful to call install on all targets

Could you give me a hint how to do it with CMake?

Another question. Do you have any experience with package managers like chocolatey, conan, vspkg? What do you think about using them instead of hand-made installation?

@markmandel
Copy link
Member

@dsazonoff can you explain what this PR does for those of us who aren't familiar with CMake + C++?

@dsazonoff
Copy link
Contributor Author

dsazonoff commented Jun 25, 2019 •
edited
Loading

@dsazonoff can you explain what this PR does for those of us who aren't familiar with CMake + C++?

Agones cpp sdk depends on protobuf and gRPC (with it's own dependencies).
This PR contains prerequisites.cmake file that is responsible for building all third party that is necessary for Agones cpp sdk (only if they are not found in system).

How it was done before: building of grpc, protobuf, zlib, cares, etc. were included directly in Agones CMakeLists.txt file. And installation of this libraries were done by custom cmake steps.

How it is done in this PR: grpc and it's dependencies are built according to cmake guidelines as pre-build step.

Goals of this PR:

  1. build grpc with it prerequisites as it was designed by grpc authors.
  2. remove custom installation of grpc/protobuf/cares/etc - installation should be done with cmake --build . --target install (it is possible only with full build of gRPC).
  3. cleanup Agones CMakeLists.txt build script.
  4. support for easy switching gRPC versions.
  5. version of sdk is picked from git tag instead of hardcode in CMakeLists.txt

CMake use find_package command to connect third party libraries and resolve it dependencies. Original problem was that gRPC requires a full build to provide a script for find_package with installation steps, but current implementation contains only partial build with manual file copying. It may be a problem for clients, if they have complex build environment.

@markmandel markmandel removed feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) labels Jun 25, 2019
@dsazonoff dsazonoff mentioned this pull request Jun 26, 2019
Closed
@markmandel
Copy link
Member

Thanks! That was super clear.

Is this ready for review/approval?

@dsazonoff
Copy link
Contributor Author

dsazonoff commented Jun 26, 2019 •
edited
Loading

@markmandel yes, it is. I just squashed commits, so there are no new changes. And here is #853, that require this PR to be merged first.
@sylvainduchesne could you approve/reject it?
Note about build time - in my opinion it's OK to have 30 min build time. 28 mins from it - it is third parties. Usually they are built only once on client side. But in next step we can change docker containers to contain pre-build gRPC.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d8cccd58-57f3-40d2-9827-ac16bf34e886

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-ad45ff4

@markmandel
Copy link
Member

I just tried this with the cpp-example, and it failed :(

https://gist.github.com/markmandel/a0f0bb703a2b1e1b33dba7721de1b7e3

Did I do something wrong?

@dsazonoff dsazonoff mentioned this pull request Jun 26, 2019
Merged
@markmandel
Copy link
Member

Cleared some of the caching, and ran it again:
https://gist.github.com/markmandel/a0f0bb703a2b1e1b33dba7721de1b7e3

Still failed.

@dsazonoff
Copy link
Contributor Author

Checking it...

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: dfdfc206-539f-4095-9121-54b944d1e2fd

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-cb361ea

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: c4f68ae2-c117-45ee-8de2-3f9db067a392

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-0c2505c

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 1b2628eb-7515-4da5-8cb9-68950e334d81

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-1b209ba

@dsazonoff
Copy link
Contributor Author

Seems that it's fixed

@markmandel
Copy link
Member

So looks like it builds, but it does not run 😞 :

➜  cpp-simple git:(pull/803) docker run --rm --network="host" gcr.io/agones-images/cpp-simple-server:0.5
/home/server/cpp-simple: error while loading shared libraries: libcares.so.2: cannot open shared object file: No such file or directory

(Can we please also push the version number of the example to 0.6 - since this is a new SDK build?)

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 79df98c1-211f-4b3b-bce7-3b0f5eb4cfdf

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: de2fffdd-3850-4c57-ac41-39942546e495

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8e6a2b24-cc5d-43c3-af46-0c7033eb20ce

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ea7a7399-d9ef-4230-9a65-63474d1e4227

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 6e8d6f43-feda-4458-861b-364718c702c7

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 597280ed-4d88-4979-95b0-83067865986c

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b1cb8c72-7501-4590-be7a-31572433a4cc

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 076cd801-1cfb-4a6e-bc91-43c96be30d31

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 95e61b1e-3ece-4932-b72c-45a9cb14483a

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-8c9c62d

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 6bcdce9a-ed8c-4dbd-a4d4-e7a0e353e82d

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-2b7c693

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 34fe8818-f7b7-4330-ab10-5bf338df79d9

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-ea92595

@dsazonoff
Copy link
Contributor Author

dsazonoff commented Jul 12, 2019 •
edited
Loading

List of fixes:

  1. build grpc with it prerequisites as it was designed by grpc authors.
  2. remove custom installation of grpc/protobuf/cares/etc - installation should be done with cmake --build . --target install (it is possible only with full build of gRPC).
  3. cleanup Agones CMakeLists.txt build script.
  4. support for easy switching gRPC versions.
  5. version of sdk is picked from git tag instead of hardcode in CMakeLists.txt

New since last review:

  1. C-ares is now built and linked statically with gRPC.
  2. Option for static/dynamic link of zlib.
  3. AGONES_THIRDPARTY_INSTALL_PATH is now set to CMAKE_INSTALL_PREFIX, but there is an option to change it (if we need separated installation for Agones SDK and it's third parties)
  4. SDK now installed to <install_path>/agones subfolder.

Problems that was fixed since last review (cpp-simple was not working):

  • c-ares dynamic library was not found if third parties are built in non-default folder
  • zlib now links statically by default (windows: it is not necessary to copy zlib.dll to a gameserver folder)

@markmandel I tested cpp-simple build&run on win/mac/linux.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b7fd6aa1-8201-491e-a1da-0843d24a4936

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/GoogleCloudPlatform/agones.git pull/803/head:pr_803 && git checkout pr_803
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-ead8495

@markmandel
Copy link
Member

Running a quick test now locally, and assuming that passes - I'll give this the approve! 👍

markmandel
markmandel approved these changes Jul 12, 2019
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

⚡ IT"S ALIVE ⚡

@markmandel markmandel merged commit bb195d3 into googleforgames:master Jul 12, 2019
@markmandel markmandel added kind/feature New features for Agones and removed kind/cleanup Refactoring code, fixing up documentation, etc labels Jul 12, 2019
This was referenced Jul 14, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvainduchesne sylvainduchesne sylvainduchesne left review comments

@markmandel markmandel markmandel approved these changes

Assignees
No one assigned
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dsazonoff @agones-bot @markmandel @sylvainduchesne