-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Initial sml version support added #4591
Conversation
Failure in build 1 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Failure in build 2 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
@mpusz Added the license https://github.com/boost-ext/sml/blob/master/LICENSE.md |
recipes/sml/all/conanfile.py
Outdated
return "build_subfolder" | ||
|
||
def validate(self): | ||
check_min_cppstd(self, "14") |
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.
It prevents to consume sml if default standard of the compiler is not 14 or above, and compiler.cppstd
not explictly set. I think that it also raises if compiler is not mainstream.
You can use this kind of pattern instead (with some adjustments on min compilers versions):
conan-center-index/recipes/capnproto/all/conanfile.py
Lines 41 to 48 in d7e0581
@property | |
def _minimum_compilers_version(self): | |
return { | |
"Visual Studio": "15", | |
"gcc": "5", | |
"clang": "5", | |
"apple-clang": "5.1", | |
} |
conan-center-index/recipes/capnproto/all/conanfile.py
Lines 57 to 63 in d7e0581
if self.settings.compiler.cppstd: | |
tools.check_min_cppstd(self, 14) | |
minimum_version = self._minimum_compilers_version.get(str(self.settings.compiler), False) | |
if not minimum_version: | |
self.output.warn("Cap'n Proto requires C++14. Your compiler is unknown. Assuming it supports C++14.") | |
elif tools.Version(self.settings.compiler.version) < minimum_version: | |
raise ConanInvalidConfiguration("Cap'n Proto requires C++14, which your compiler does not support.") |
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 am not sure how it helps here? The only issue with my usage of check_min_cppstd(self, "14")
is that it might not work for some strange niche compiler and the user will be forced to set cppstd for it explicitly. This eventually makes it work for any compiler.
Adding the rest of the suggested code does not help here as tools.check_min_cppstd(self, 14)
will throw an Exception for an unknown compiler anyway so the rest of the code will not be executed, right?
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.
It allows to use the recipe even if cppstd (which is an experimental feature) is not set.
If you look at https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/4591/2/sml/1.1.3//summary.json, it throws for all apple-clang versions tested in CI.
The suggested code:
- throws if cppstd < 14 is explicitly requested
- throws if cppstd is not set or >= 14, and the compiler version in profile is known for not supporting C++14
- displays a warning if cppstd is not set or >= 14, and we don't know if the compiler supports C++14
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 do not think that we should verify the C++ standard version based on the compiler's version. If this is the case we need a better utility in Conan tools. It is just too verbose and error-prone.
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.
But if you prefer, I can rename validate
to configure
(this change will probably be reverted for Conan 2.0 later on) and add if self.settings.compiler.get_safe("cppstd"):
.
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.
Learned this the hard way ;-) I think that now everything is fine?
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.
For my tracking purposes conan-io/conan#8002
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.
Please follow the suggestions above, this is a requirement because of a technical limitation of CCI... so for now we need to make sure this is merged in.
Thanks for understanding 😄
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.
Sure, I changed to configure()
usage 4 days ago.
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.
Yes but you have to implement the full solution proposed above (with explicit compiler versions).
Failure in build 3 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
All green in build 4 (
|
Failure in build 5 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
All green in build 6 (
|
Hi, |
recipes/sml/all/conanfile.py
Outdated
return "build_subfolder" | ||
|
||
def validate(self): | ||
check_min_cppstd(self, "14") |
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.
Please follow the suggestions above, this is a requirement because of a technical limitation of CCI... so for now we need to make sure this is merged in.
Thanks for understanding 😄
#include <boost/sml.hpp> | ||
#include <cassert> | ||
|
||
namespace sml = boost::sml; |
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.
Is there any way we can make a cross platform test package? it does not need to do anything just use those headers should be sufficient?
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.
@krzysztof-jusiak, could you please share a simpler cross-platform test example?
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.
Misclicked.
Co-authored-by: Chris Mc <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
Failure in build 7 ( An unexpected error happened and has been reported. Help is on its way! 🏇 |
All green in build 8 (
|
Failure in build 9 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Failure in build 10 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
All green in build 11 (
|
Co-authored-by: SpaceIm <[email protected]>
All green in build 12 (
|
@krzysztof-jusiak Could you explain why this recipe can't be prefixed with |
It's to avoid confusion with official boost libraries. It's advised not to use boost name in the user-facing packages until the review/acceptance will happen. |
Specify library name and version: sml/1.1.3
conan-center hook activated.