-
Notifications
You must be signed in to change notification settings - Fork 103
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
Check for required C++ features during initial build (std::format, etc) #3479
base: main
Are you sure you want to change the base?
Conversation
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.
I'm fairly happy with this approach.
I don't think we'll keep the required feature list up to date (and this is missing a bunch of features we use, like operator <=>
and std::binary_semaphore
), but when people run into problems and pop up in the bugtracker we can reactively encode that knowledge.
@@ -13,7 +13,7 @@ concurrency: | |||
|
|||
jobs: | |||
Run: | |||
runs-on: ubuntu-latest | |||
runs-on: ubuntu-24.04 |
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.
This change seems incidental. Is this a required part of the PR, or a remnant of a previous test?
@@ -7,7 +7,7 @@ version: 2 | |||
|
|||
# Set the version of Python and other tools you might need | |||
build: | |||
os: ubuntu-22.04 | |||
os: ubuntu-24.04 |
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.
Likewise, is this required?
|
||
include_guard(GLOBAL) | ||
|
||
#include (CheckSymbolExists) |
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.
Probably don't need to leave commented-out remnants of testing 😉
# Header containing the available C++ standard library features | ||
set(FILES "version") | ||
|
||
#__CHECK_SYMBOL_EXISTS_IMPL(RequireCXXFeature.cxx "${SYMBOL}" "${FILES}" "${VARIABLE}") |
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.
Also commented out remnant of testing?
|
||
include_guard(GLOBAL) | ||
|
||
#include (CheckSymbolExists) |
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.
And here!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3479 +/- ##
==========================================
+ Coverage 76.95% 76.97% +0.01%
==========================================
Files 1080 1080
Lines 69380 69380
==========================================
+ Hits 53394 53404 +10
+ Misses 15986 15976 -10 ☔ View full report in Codecov by Sentry. |
Per the discussion in #3472, this is an alternative approach of informing users that their C++ compiler and standard library doesn't support certain required features. It was born out of my challenges getting mainline Mir built and running on Pop!_OS (Ubuntu 22.04 LTS) instead of Ubuntu 24.04 LTS.
Admittedly it's a "big ask" of any project to track used language features, but C++ is an unusual case, for how closely your compilers are tied to your Linux distro.
To make managing the language feature list serve a dual purpose, and in lieu of a coding style guide, I proposing treating these additions to the
CMakeLists.txt
file as a master list of recommended C++ features for all contributors. That doesn't mean adding every C++20 feature to the list, just the ones you want contributors to use.Under the hood, I wrote some macros that wrap
CMAKE_CXX_COMPILE_FEATURES
andcheck_cxx_symbol_exists
in a way that should be both forward and backward compatible with different versions of cmake. It relies on C++20 feature testing, using the exact names from the specification. You can find a handy list of C++ feature macro names here:https://en.cppreference.com/w/cpp/utility/feature_test
NOTE: cmake refers to C++ as
cxx
, but C++ feature testing refers to it ascpp
.Usage looks something like this.
CMakeLists.txt
If your tools meets the requirements, nothing out of the ordinary will occur. If you don't meet them, you will get an output similar to this:
This is a proof-of-concept, but it should be fully functional for the use case described. The next step would be to dig into the source code and populate the list of recommended C++ features.
Polyfilling features
For completeness, I included cmake macros for both
require_cxx_feature
andcheck_cxx_feature
. In practicerequire_cxx_feature
is probably all that's needed, but in case you wanted to polyfill a C++ feature or standard library,check_cxx_feature
can be used instead.Nitty gritty
require_cxx_feature
andcheck_cxx_feature
are a simpler interface to the standard cmake moduleCheckCXXSymbolExists
. The compilers define the language feature symbols (i.e.__cpp_FEATURENAME
), and C++20 requires that the standard library provide a header calledversion
defining all library features (i.e.__cpp_lib_FEATURENAME
).GCC 11
for example supports many C++20 features, but the standard library shipped alongside it was incomplete. Theversion
header exists, but per the examples above, theformat
header and symbol__cpp_lib_format
do not.CheckCXXSymbolExists
stores the variable given in the final argument in the cmake cache. Once cached, the check will not be done again until the cache is purged.require_cxx_feature
purges that cache entry on failure, so the test will be run again when cmake is run again. Once it succeeds, likeCheckCXXSmybolExists
it will not be checked again until the cmake cache is purged.