-
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
andreasbuhr-cppcoro: Conan 2.0 support #20611
Conversation
I detected other pull requests that are modifying andreasbuhr-cppcoro/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
Hi @Ahajha thanks a lot for your PR, we really appreciate it! :) Just checked that there's a similar PR (I thought my first review didn't go thru!) for this recipe, maybe we can work to bring them both together with their changes? |
@RubenRBS I didn't even notice there was another PR for this until I had already pushed it, but it seems this one will be a bit easier since it has a few more months of changes/fixes. I can definitely incorporate the rest of that branch's changes. |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 18ef9c1andreasbuhr-cppcoro/cci.20210113@#cd2c14706331779efaea159f160e1418
|
Getting a C5231 on MSVC in the test package, which has almost no documentation online, I can't visually tell what's causing it, and I can't reproduce it on my machine. So for now trying to remove 20210113 and see if it's fixed upstream. |
This comment has been minimized.
This comment has been minimized.
Not quite ready yet:
|
This comment has been minimized.
This comment has been minimized.
Looking at the changes between cci.20210113 and the latest, it's mostly very small bugfixes (including the windows build issue), so I imagine it won't cause any breakages for people to upgrade. Thus, I think we can just leave only the latest version. If someone needs a previous version we can revisit. With that note, this should be ready for another review once CI passes. |
This comment has been minimized.
This comment has been minimized.
@RubenRBS This is ready for review (not sure if there's another process to signaling ready-ness) |
Pinging works, thanks :) (If not, we usually come around the PR eventually after a few days) |
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! Somall questions, but looks nice
@RubenRBS Can you please take another look at this PR to see if there's any more required changes? |
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. I have a concern with Clang >= 16. I've proposed a solution, and I'm interested in hearing what you think about it :)
"gcc": "10", | ||
"clang": "8", | ||
"apple-clang": "10", | ||
} | ||
|
||
@property | ||
def _min_cppstd(self): | ||
return 17 |
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 will be an issue for compilers that only have coroutines support for C++20 enabled. This is the case of clang >= 16, where only having -std=c++20
enables coroutines. With this check, CCI will only try to build for C++ 17. Maybe a solution is to force C++20 as a min required version if the other flag checks have not succeeded?
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.
Maybe we could just update this value to be
return 20 if self.settings.compiler == "clang" and self.settings.compiler.version >= 16 else 17
?
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.
There's also a funny edge case that I just ran into: When using libstdc++ with clang, it always required C++20. With libc++, it allows C++17 actually through clang 16, but fails with 17. I'll double check all my tests, but assuming that's correct I'll update the checks.
@RubenRBS A couple things I found while doing some more extensive clang testing:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@RubenRBS I think this is ready for another look (I don't see the Conan v2 pipeline though, odd) |
Conan v1 pipeline ✔️All green in build 16 (
Conan v2 pipeline ✔️
All green in build 18 (
|
@RubenRBS Can you take another look at this one? |
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.
LGTM
Specify library name and version: andreasbuhr-cppcoro/all
I noticed the recipe previously had a single component defined, which seemed unnecessary, so I removed it. If anyone might know why that was there I can re-add it.
Closes #20564
Closes #16621