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

cmake: require CMake v3.10.2 #29291

Merged
merged 2 commits into from Aug 3, 2019
Merged

Conversation

tchaikov
Copy link
Contributor

since we dropped the support of xenial, we now have the luxury of using
newer CMake!

so in this change,

  • bump up the required version to v3.10.2
  • cleanups

Signed-off-by: Kefu Chai kchai@redhat.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

this allows some nice cleanups, thanks!

@cbodley
Copy link
Contributor

cbodley commented Jul 24, 2019

from jenkins,

PRETTY_NAME='Ubuntu 16.04.6 LTS'
CMake 3.10.2 or higher is required. You are running version 3.5.1

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 24, 2019

@alfredodeza @djgalloway can we label the jenkins slave so that only bionic and centos7 slaves are used for testing master? currently, we choose "huge && (centos7 || trusty) && rebootable" for testing PRs. see

https://github.com/ceph/ceph-build/blob/ba96e7971db5d57cf3ca8e11c7ee71cfd84bd2b7/ceph-pull-requests/config/definitions/ceph-pull-requests.yml#L6

@tchaikov tchaikov added the DNM label Jul 24, 2019
@djgalloway
Copy link

can we label the jenkins slave so that only bionic and centos7 slaves are used for testing master? currently, we choose "huge && (centos7 || trusty) && rebootable" for testing PRs.

We don't actually have any Bionic slaves. Any Ubuntu packages are built on a Xenial slave using pbuilder. See https://github.com/ceph/mita/blob/master/deploy/playbooks/roles/common/templates/prod_nodes.py.j2#L81-L92

Notice a huge && trusty && rebootable slave is using 'image_name': 'Ubuntu 16.04'.

@alfredodeza
Copy link
Contributor

what we need is to re-do the labelling and cleanup, because the current way doesn't help and it was the compromise to keep things running when things migrated from the very first jenkins iteration. Having a label that says 'bionic' when it really isn't is super confusing :(

@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 2, 2019

@djgalloway @alfredodeza is it possible for us to use 'Ubuntu 18.04' for 'trusty_huge' ? or better off defining a new type of node 'bionic'? and then, we could, as @alfredodeza suggested, re-label the slaves and clean up the templates a little bit?

to enable us to build on xenial, install newer cmake.
cmake 3.10.2 is the version offered by bionic.

with this change, we can safely require cmake 3.10.2 in our cmake
script. as EPEL7 offers cmake 3.13

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 2, 2019

retest this please.

@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 2, 2019

changelog

  • s/VERSION/PROJECT_VERSION/
  • add a commit to ensure cmake 3.10.2.

@cbodley could you take another look?

set(CMAKE_C_EXTENSIONS ON)
set(C_STANDARD_REQUIRED ON)
endif()
set(CMAKE_CXX_STANDARD 17)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this helps fmt to use C++17 instead of falling back to C++11. which fails the build.

@alfredodeza
Copy link
Contributor

It is going to take a good chunk of effort to have true Bionic nodes, starting with Python 3 support which none of our tooling is ready for

since we dropped the support of xenial, we now have the luxury of using
newer CMake! and by using CMake 3.10.2, we can prevent libfmt from
assuming that we are using C++11, and hence set `CMAKE_CXX_STANDARD` to
11, which will literally append `-std=gnu++11` to `CMAKE_CXX_FLAGS`.
the last `-std` option passed to `g++` takes precendence.
since we've switched over to C++17, and we are using C++17 features.
so, using cmake older than 3.8 breaks the build. because it is CMake 3.8
which stared support `CMAKE_CXX_STANDARD` 17.

- for bionic: https://packages.ubuntu.com/bionic/cmake : 3.10.2
- for CentOS7:
https://dl.fedoraproject.org/pub/epel/7/x86_64/Packages/c/ : 3.13.5

so in this change,

* bump up the required version to v3.10.2
* cleanups to wipe out the workaround for lower CMake versions
* use `PROJECT_VERSION` defined by `project()` command instead of
  `VERSION` explicitly defined.

Signed-off-by: Kefu Chai <kchai@redhat.com>
Copy link
Member

@neha-ojha neha-ojha left a comment

Choose a reason for hiding this comment

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

looks fine to me

@tchaikov tchaikov merged commit d20a13b into ceph:master Aug 3, 2019
@tchaikov tchaikov deleted the wip-cmake-cleanups branch August 3, 2019 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants