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

Changed cmake required version to 3.5 to work on more machines #903

Merged
merged 3 commits into from
Jul 16, 2019

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Jul 13, 2019

The current cmake files don't actually require 3.13, so lowering the version they claim to require will allow the cmake files to work on more machines. Related to #718

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 90c740ae-97a1-48e1-ba60-82eb2297456b

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

@dsazonoff
Copy link
Contributor

I don't think that cmake version should be downgraded. It is very easy to upgrade it. And SDK supports docker build, so it doesn't matter for a machine what version to have.
There was a request from @markmandel to silent CMake output and there was a bug in older versions of CMake when output was not silenced for some platforms. If necessary, I can find an issue in cmake bugtracker. But it is not critical.

Don't forget to patch 'cpp/cmake/clang-verify.in' file and Dockerfile that are responsible for SDK builds (installation of CMake).

@devjgm
Copy link
Contributor Author

devjgm commented Jul 13, 2019

I don't think that cmake version should be downgraded. It is very easy to upgrade it.

My distro is using a debian mirror that's slightly behind, and the newest cmake I can get is 3.12.1. So my only choice is to either: (A) download cmake from kitware.com (yuck), or (B) edit the "cmake_minimum_required" version to something slightly lower, which works fine.

FTR, we choose to require cmake 3.5 for https://github.com/googleapis/google-cloud-cpp because it's easily available on a large variety of important Linux distros, and it actually has most of the major modern features that people want/need with cmake.

And SDK supports docker build, so it doesn't matter for a machine what version to have.

Are there instructions for that? Because running make, as I did, used my locally installed cmake and did not use docker.

Don't forget to patch 'cpp/cmake/clang-verify.in' file and Dockerfile that are responsible for SDK builds (installation of CMake).

I'm sorry, but I don't see any mention of clang-verify.in. Can you please provide a few more details or point me at the documentation? Thanks.

@dsazonoff
Copy link
Contributor

Now I agree about downgrading cmake version.

Are there instructions for that? Because running make, as I did, used my locally installed cmake and did not use docker.

You may look here. Or here. But I usually build everything locally, without docker.
I was talking about this Dockerfile. I was wrong in my previous comment. I think we should downgrade cmake version in CMakeLists.txt files, but keep use v3.14 in docker build. So minimal supported version will be 3.5, but cloud build will use 3.14.

I'm sorry, but I don't see any mention of clang-verify.in

I noticed that you created your PR before #855 was merged.

Please, update from master and fix cmake version in clang-verify too.
Other things are fine.

@devjgm
Copy link
Contributor Author

devjgm commented Jul 14, 2019

Updated. Requiring 3.5 but using 3.14 in Docker sounds like a good idea to me.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8f53308f-26f6-44db-b9f3-3438b9cdc88d

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/903/head:pr_903 && git checkout pr_903
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-c801d68

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 23d34505-5c56-4eae-ba65-8fe1feaf5775

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/903/head:pr_903 && git checkout pr_903
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-1389a24

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.

❇️

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: df813d7e-2fd3-4d2b-9e03-082346157802

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/903/head:pr_903 && git checkout pr_903
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-b069fd9

@markmandel markmandel merged commit 6429d35 into googleforgames:master Jul 16, 2019
@markmandel markmandel added this to the 0.12.0 milestone Jul 16, 2019
@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 Jul 16, 2019
@roberthbailey
Copy link
Member

@devjgm
Copy link
Contributor Author

devjgm commented Jul 22, 2019

I think so, yes. I didn't know "examples/cpp-simple" existed, otherwise I'd have updated it. But I see no reason for it to be different. Thanks for pointing this out.

@devjgm devjgm deleted the lower-cmake-version branch January 23, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants