Add cmake support #174
Add cmake support #174
Conversation
Add cmake support
- adding doctest discovery (doctest.cmake)
test discovery
FindCoroutines.cmake tried to figure out the correct flags to use coroutines. However, with MSVC 16.8 this is impossible. MSVC 16.8 supports both the coroutines-ts with its experimental headers and experimental namespace as well as the C++20 coroutines. The user can select the coroutines-ts with the /await flag and the C++20 coroutines with /std:c++latest. Only the user can know what she wants. This patch changes the logic in FindCoroutines.cmake to basically: If it does not compile without (/await|-fcoroutines-ts), try adding (/await|-fcoroutines-ts) to the compiler flags.
- tests-main library changed to STATIC - MSVC dlls would require to do extra import/export effort
Add cmake support
| target_compile_definitions(cppcoro PUBLIC ${compile_definition}) | ||
| target_compile_options(cppcoro PUBLIC ${compile_options}) | ||
|
|
||
| find_package(Coroutines COMPONENTS Experimental Final REQUIRED) |
There was a problem hiding this comment.
This fails with VS 2019 16.8.2. The find_package(Coroutines ...) reports this error:
CMake Error at cmake/FindCoroutines.cmake:272 (message):
Cannot compile simple program using std::coroutines. Is C++17 or later activated?
Since 3 lines above you specifiy for this target c++20/latest, you should activate it also for the check in the FindCoroutines. This does the trick:
| find_package(Coroutines COMPONENTS Experimental Final REQUIRED) | |
| set(CMAKE_CXX_STANDARD 20) | |
| find_package(Coroutines COMPONENTS Experimental Final REQUIRED) |
With this change, the CMake generation is OK, but now you will get compilation errors, because now you are using the 'real' c++20 coroutines and not the TS ones.
C1189 The <experimental/coroutine> and <experimental/resumable> headers are only supported with /await and implement pre-C++20 coroutine support. Use for standard C++20 coroutines.
The solution is thus to merge also the changes from https://github.com/andreasbuhr/cppcoro/tree/unify_experimental_includes
The master branch from andreasbuhr, that combines this various changes, generates and compiles fine, after the patching with the suggested change set(CMAKE_CXX_STANDARD 20) before find_package(Coroutines ...)
There was a problem hiding this comment.
Unfortunately, the fix you propose is not possible. As you pointed out, MSVC 2019 supports both the coroutine-ts and C++20 coroutines. If the user wants to build her project with the coroutine-ts, also cppcoro has to be built with the coroutine-ts. Hardcoding CMAKE_CXX_STANDARD to "20" would make that impossible.
Only the user knows whether she wants the coroutine-ts and C++17 or C++20 coroutines. There is no way of detecting that. So there is no other way than to ask the user what she wants when building. This is what the error message "Cannot compile simple program using std::coroutines. Is C++17 or later activated?" does.
cppcoro needs to be build using either "cmake -DCMAKE_CXX_STANDARD=17" OR "cmake -DCMAKE_CXX_STANDARD=20". This, however, needs to be documented better.
Would you agree with my analysis?
There was a problem hiding this comment.
I thought that this decision (coroutine-ts and C++17 or C++20 coroutines) is already hardcoded 3 lines above target_compile_features(cppcoro PUBLIC cxx_std_20). I will test your proposal with the invocation of cmake using the CMAKE_CXX_STANDARD define. But I have a bad gut feeling, especially since I hoped that your version would once also be incorporated in vcpkg. The current port on vcpkg has currently the TS version hardcoded. I do not know if with vcpkg you can let the user choose the cxx_standard version.
There was a problem hiding this comment.
Thanks a lot for poiniting this out:
target_compile_features(cppcoro PUBLIC cxx_std_20)
this has to be fixed.
I hope I can add C++17/coroutine-ts to the Github actions CI in andreasbuhr/cppcoro in the coming weekend, then we catch these things.
Thanks also to pointing to the issue with vcpkg. I will look into that in the coming weekend.
There was a problem hiding this comment.
Only the user knows whether she wants the coroutine-ts and C++17 or C++20 coroutines.
The devs who already jumped on the experimental coroutines bandwagoon are leaving on the edge anyway, ahead of HEAD, taking some risks and enjoing the latest and greatest :-) My personal opinion, is that the old stuff in this area can be deprecated, as soon as you have new working compiler version officially available.
There was a problem hiding this comment.
I removed the statement now from this pull request.
About your suggestion about deprecation: I would agree, but I'd like to have one release of cppcoro which works with the coroutine-ts and C++17 first. After the release, we can remove the C++17 / coroutine-ts support. That way we get it out of the way in the long run and at the same time we can direct people who need coroutine-ts support to that release.
lib/CMakeLists.txt contained a target_compile_features(cppcoro PUBLIC cxx_std_20) which forced cppcoro to be built with C++20. However, the user might want to compile in C++17 mode. This statement made it impossible. This patch removes this statement.
|
This pull request is now open for two month. I now stop maintaining this pull request. From now on, I will continue to maintain the master branch in https://github.com/andreasbuhr/cppcoro. There we have CMake support, a CI based on Github Actions, and support for the latest compilers. |
Pull request #110 by @mmha was opened 1.5 years ago, but was never finished. @dutow later based his fixes for gcc on it. In #169, CMake support was again proposed by @Garcia6l20.
This pull request should become a polished pull request which incorporates material from all those pull requests to add CMake support to cppcoro.
Any help or review is greatly appreciated. Especially, help in testing with MSVC would be great.
To help improve this pull request, please fork https://github.com/andreasbuhr/cppcoro/tree/add_cmake_support and create a pull request toward this branch.