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: check gcc version not release date for libstdc++ saneness #18938

Merged
merged 3 commits into from Nov 29, 2017

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Nov 15, 2017

this is a spin-off #18863, since it is still pending on further discussion. i hope part of it can be merged sooner.

otherwise, the default gcc will be used, and the $CMAKE_C_COMPILER
passed to the outer CMakeLists.txt won't kick in. moreover, if the
building script (ceph.spec for instance) could set the $PATH, and
expect that the CMakeLists.txt will use the toolchain executables
in the $PATH to build Ceph, distutils will continue using the default
$CC for linking the python bindings, on UNIX it will be gcc in the
new shell's $PATH, because we are using `install(CODE "... execute_process(
...))` for installing the python bindings. apparently, this is not
expected. because the new shell's $PATH is very likely different
from the one changed by the building script. to address this, we
should always specify the `$LDSHARED` env var explicitly.

also, pass env vars using `ENV{}` instead of the `env` command to
workaround the issue of https://cmake.org/pipermail/cmake/2015-December/062216.html,
because it's not straightforward to set environment variables with
spaces in the them using cmake. and because one cannot use add_custom_target()
in the script mode of cmake. this leave me only limited options to
fix this issue.

Signed-off-by: Kefu Chai <kchai@redhat.com>
there is chance that the release date of a minor or patch version of
libstdc++/gcc is *greater* than that of a major version. so this renders
the existing approach to check the __GLIBCPP__ useless. let's check the
gcc version instead. it's far from a perfect solution. but it's good
enough to cover most cases. assuming that most users use gcc with
libstdc++ comes with it. instead of using 1) gcc with a newer libstdc++,
or 2) clang with a old libstdc++.

Signed-off-by: Kefu Chai <kchai@redhat.com>
see https://gitlab.kitware.com/cmake/cmake/issues/17381 and
https://gitlab.kitware.com/cmake/cmake/commit/a8be8b1b54fe1922a1d1fc0365c3ae5c918b6654,
so before the updated cmake is released and packaged. we should
add this setting.

Signed-off-by: Kefu Chai <kchai@redhat.com>
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.

untested but looks good

@tchaikov
Copy link
Contributor Author

@tchaikov tchaikov merged commit 12dc573 into ceph:master Nov 29, 2017
@tchaikov tchaikov deleted the wip-cmake branch November 29, 2017 07:38
xiexingguo pushed a commit to ceph/ceph-ci that referenced this pull request Jan 17, 2018
    ceph/ceph#19548

cmake: check gcc version not release date for libstdc++ saneness
cmake: bail out if GCC version is less than 5.1

    ceph/ceph#18938
    ceph/ceph#19344

build/ops: use devtoolset-7 on centos/rhel-7
ceph.spec: use devtoolset-6-gcc-c++ on aarch64

    ceph/ceph#18863
    ceph/ceph#19341

install-deps: use DTS-7 on aarch64 and only download mirrored package indexes

    ceph/ceph#19645

Signed-off-by: luo.runbing <luo.runbing@zte.com.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants