-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[boost] Check for C++20 compiler support when building Cobalt #24149
[boost] Check for C++20 compiler support when building Cobalt #24149
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
🤖 Beep Boop! This pull request is making changes to 'recipes/boost//'. 👋 @grafikrobot @Hopobcn @jwillikers @paulharris you might be interested. 😉 |
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.
Great catch :)
Conan v1 pipeline ✔️All green in build 1 (
Conan v2 pipeline ✔️
All green in build 1 (
|
Hooks produced the following warnings for commit 17e9b7aboost/1.85.0@#db9b7d91e0ad8ba0b5878eb24601edb7
boost/1.81.0@#44f903c7690d66225351d90b738787bc
boost/1.73.0@#5375ddc1568645bb2dee98a7e2a8dbdf
boost/1.84.0@#b5dcda38cd49cbfedbb80dd939115ea6
boost/1.82.0@#2beb8afb021d92523db42e16db2dc946
boost/1.79.0@#612c9e8e2dc8103669fa626ad240ca3d
boost/1.80.0@#c0dced27fd872f001b859837478ae355
boost/1.83.0@#99a01316f748f2c9fcd163408217b5a1
boost/1.78.0@#76ab0d03d2bab6a37a393e76b96de1fe
boost/1.74.0@#bbbd7847c6caeabba3bddf2fbdf7664a
boost/1.71.0@#1a3954d4770e3fb7b216e73ff29f2387
boost/1.75.0@#e5ea0369ba817cd3c5b8f0c3e0c2f285
boost/1.72.0@#fdf235e12b801fd7a378ed217e6950e6
boost/1.76.0@#100c13ad2a8317275f84fcda9d32be42
boost/1.77.0@#1ff54f24e525d4d1faafce3237ca9391
|
def _has_cppstd_20_supported(self): | ||
cppstd = self.settings.compiler.get_safe("cppstd") | ||
if cppstd: | ||
return valid_min_cppstd(self, 20) |
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 suspect that this isn't sufficient to exclude all invalid compilers. valid_min_cppstd
returns True
for gcc 8, gcc 9, gcc 10, clang 10, clang 11 clang 12 and clang 13 - all of which have partial support for C++20, but the features that cobalt relies on aren't implemented (libstdc++, this may say more about the gcc version installed on the docker images for clang than the clang versions). Unfortunately none of these compiler versions are in the test matrix (or don't have cppstd=20 set), so we're not seeing this reflected in CI
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.
@uilianries @RubenRBS is this not something the conan team are concerned about?
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.
@samuel-emrys It's hard to track partial features implemented by compiler as we don't run a code to validate it. Still, if you think is something to be improved, please, open an issue to https://github.com/conan-io/conan/issues.
About Boost, we have a other checks related to the compiler version too, not only valid_min_cppstd. Now, users are suppose the be apart of their compilers, and what features they support. Here we have all those checks to avoid breaking when building, so we can inform in advance.
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.
@uilianries maybe I'm missing it - I can't see where compiler versions for cobalt are checked for..
The changes you've introduced here only fix part of the problem with cobalt - checking for the required cppstd is important, but gcc < 11 and clang < 14 will return true for your check, and will fail because they miss the necessary features to build cobalt. The result is that our pipelines for these compiler versions fail because boost is trying to build with cobalt which is not supported for these compiler versions.
The problem here is just that cobalt is building by default when valid_min_cppstd(self, 20)
, but the logic should be Version(self.settings.compiler.version) >= self._min_cobalt_compiler_version() and valid_min_cppstd(self, 20)
Perhaps this check is better placed in config_options
than the line I've commented on though.
Specify library name and version: boost/1.85.0
The Boost Cobalt has been introduced in Boost 1.84.0 (changelog and requires C++20 support for coroutines (Jamfile).
The current recipe for Boost only validates C++11 and C++14 (Boost math) compilers, but not C++20 (Cobalt). This PR introduces a new check to validate when building Cobalt: The cppstd should be C++20, or the compiler should be new enough (Conan 1.x).
fixes #24148