-
Notifications
You must be signed in to change notification settings - Fork 440
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
Improve C++20 detection with CMake 3.12+ and gcc-9/c++20 support fixes #860
Conversation
CMake v3.12 introduced C++20
…pen-telemetry/opentelemetry-cpp into maxgolov/fix_cmake_cxx_standard
Codecov Report
@@ Coverage Diff @@
## main #860 +/- ##
=======================================
Coverage 95.50% 95.50%
=======================================
Files 156 156
Lines 6620 6620
=======================================
Hits 6322 6322
Misses 298 298
|
@ThomsonTan - if you can have a quick look? I think this PR would be valuable addition to Lalit's CI PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this.
@@ -107,7 +107,7 @@ if(WITH_STL) | |||
add_definitions(-DHAVE_CPP_STDLIB) | |||
add_definitions(-DHAVE_GSL) | |||
# Require at least C++17. C++20 is needed to avoid gsl::span | |||
if(CMAKE_VERSION VERSION_GREATER 3.18.0) | |||
if(CMAKE_VERSION VERSION_GREATER 3.11.999) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use VERSION_GREATER_EQUAL 3.12.0
to avoid using the crafted version number?
@ThomsonTan - also I have addressed your review comments in #855. Please see if they are fine. Thanks |
Fixes #859
Our logic was assuming that it's only introduced in 3.18.x , and it would break in Ubuntu-20.04 which has a compiler with C++20, but its CMake version is only 3.16 (less than 3.18).. Thus, it was always falling back to C++17 language standard.
nostd::variant
projection - it was missingbad_variant_access
. And the second one was usingstd::strlen
without#include <cstring>
. Both have now been resolved. Interestingly enough, those issues did not affect vs2019 in c++20/c++latest 😄How to test
Our build system "developer-focused" part for Linux and Mac (
/tools/
) already has the build scripts to run the build as follows:nostd:
tools/build.sh
stdlib:
tools/build.sh stdlib -DWITH_STL=ON
That would produce the binaries under
out/x86_64-linux-gnu-gcc-9/[nostd|stdlib]
. Default build without any parameters - producesnostd
flavor. And if custom parameters passed to CMake - these allow to produce the build with any configuration name (1st argument is configuration name). For example, the build with Standard Library types, which in turn always turns on C++20 , if available. If C++20 is not available, then it falls back to C++17.Build script
tools/build.sh
- allows to build different configurations side-by-side in one workspace, with build artifacts for different configurations produced side-by-side in a single output directory, as follows:It's also possible to use WSL2 and Windows machine to host multiple docker images with different compilers. All producing the output for subsequent packaging from one single binary artifacts directory a.k.a. multiple docker / single filesystem build farm, side-by-side, one workspace - many containers - separate output directories for artifacts built by different compilers. Neat, isn't it?