Skip to content
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

Added CMake build system #110

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Added CMake build system #110

wants to merge 1 commit into from

Conversation

mmha
Copy link

@mmha mmha commented Apr 22, 2019

  • Added a CMake build scripts + packaging
  • Updated to README

RFC:
Should the CMake package check for coroutine support when being invoked by find_package()?

TODO:

  • Update .travis.yml


add_library(cppcoro::coroutines INTERFACE IMPORTED)
if(Coroutines_SUPPORTS_MS_FLAG)
target_compile_options(cppcoro::coroutines INTERFACE /await)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under msvc optimised x64 builds we also want to add the "/await:heapelide" flag to take advantage of the coroutine frame heap-elision optimisations.

include(FindPackageHandleStandardArgs)

check_cxx_compiler_flag(/await Coroutines_SUPPORTS_MS_FLAG)
check_cxx_compiler_flag(-fcoroutines-ts Coroutines_SUPPORTS_GNU_FLAG)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that since the adoption of coroutines into C++20 that clang (trunk?) now also enables coroutines when -std=c++2a is enabled.

@lewissbaker
Copy link
Owner

Thanks for the PR and especially for adding docs!

I've added a few minor notes.
I won't be able to comment much on the CMake style/structure, though, as I'm a CMake newbie.

@denchat
Copy link
Contributor

denchat commented May 21, 2019

I tried build with LLVM clang-cl. It doesn't compile cppcoro/file.hpp which include experimental/filesystem due to anonymous struct declaration struct char8_t;

[19/69] Building CXX object lib\CMakeFiles\cppcoro.dir\read_only_file.cpp.obj
FAILED: lib/CMakeFiles/cppcoro.dir/read_only_file.cpp.obj
C:\PROGRA~1\LLVM\bin\clang-cl.exe  /nologo -TP -Dcppcoro_EXPORTS -IC:\Users\User\AppData\Roaming\cppcoro-master\include -Xclang -std=c++2a -Xclang -fno-char8_t -Wno-gnu-anonymous-struct -fms-compatibility-version=19.20.27508 --target=x86_64-pc-windows-msvc19.20.27508 /EHsc /Zc:__cplusplus /MD /O2 /Ob2 /DNDEBUG   /await -std:c++latest /showIncludes /Folib\CMakeFiles\cppcoro.dir\read_only_file.cpp.obj /Fdlib\CMakeFiles\cppcoro.dir\ -c C:\Users\User\AppData\Roaming\cppcoro-master\lib\read_only_file.cpp
clang-cl: warning: argument unused during compilation: '/await' [-Wunused-command-line-argument]
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\lib\read_only_file.cpp:6:
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\include\cppcoro\read_only_file.hpp:8:
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\include\cppcoro/readable_file.hpp:8:
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\include\cppcoro/file.hpp:18:
C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.20.27508\include\experimental/filesystem(45,13): error: declaration of anonymous struct must be a definition
            struct char8_t; // flag for UTF8
            ^
1 error generated.

@lewissbaker
Copy link
Owner

I tried build with LLVM clang-cl. It doesn't compile cppcoro/file.hpp which include experimental/filesystem due to anonymous struct declaration struct char8_t;

@denchat This seems like an issue with mixing clang-cl and the MSVC standard library headers rather than with cppcoro. Has it been reported to Microsoft?
Would the same issue occur if cppcoro were to use <filesystem> instead of <experimental/filesystem>?

@denchat
Copy link
Contributor

denchat commented May 30, 2019

@denchat This seems like an issue with mixing clang-cl and the MSVC standard library headers rather than with cppcoro. Has it been reported to Microsoft?
Would the same issue occur if cppcoro were to use <filesystem> instead of <experimental/filesystem>?

@lewissbaker
I looked at <filesystem>. It is a very short header and it includes <experimental/filesystem>. It seems all microsoft implementation defined in <experimental/filesystem> as of VC++ 2019.


After more googling around, I found that the problematic line of c++ prohibited non-definition anonymous struct in <experimental/filesystem> of msvc standard library:

struct char8_t;

uses the Microsoft non-standard extension, which is turned on by default by cl.exe and can be explitcitly activated by /Ze

Compiler Warning (level 4) C4201 - nonstandard extension used : nameless struct/union
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4201?view=vs-2019

Unfortunately, rightnow it seems clang-cl.exe don't let us use /Ze and it seems to ignore the flag.

C:\Users\User\AppData\Roaming\cppcoro>ninja -j 2
[19/69] Building CXX object lib\CMakeFiles\cppcoro.dir\writable_file.cpp.obj
FAILED: lib/CMakeFiles/cppcoro.dir/writable_file.cpp.obj
C:\PROGRA~1\LLVM\bin\clang-cl.exe  /nologo -TP -Dcppcoro_EXPORTS -IC:\Users\User\AppData\Roaming\cppcoro-master\include -Xclang -std=c++2a -Xclang -Wno-gnu-anonymous-struct -Wno-unused-command-line-argument -fms-compatibility-version=19.20.27508 --target=x86_64-pc-windows-msvc19.20.27508 /EHsc /Zc:__cplusplus /Ze /MD /O2 /Ob2 /DNDEBUG   /await -std:c++latest /showIncludes /Folib\CMakeFiles\cppcoro.dir\writable_file.cpp.obj /Fdlib\CMakeFiles\cppcoro.dir\ -c C:\Users\User\AppData\Roaming\cppcoro-master\lib\writable_file.cpp
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\lib\writable_file.cpp:6:
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\include\cppcoro/writable_file.hpp:8:
In file included from C:\Users\User\AppData\Roaming\cppcoro-master\include\cppcoro/file.hpp:18:
C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.20.27508\include\experimental/filesystem(45,13): error: declaration of anonymous struct must be a definition
            struct char8_t; // flag for UTF8
            ^
1 error generated.

Using clang-cl.exe is still a road-block, at least when we are including <cppcoro/file.hpp>, if

  • MSVC <filesystem> and <experimental/filesystem> still use anonymous struct and the compiler extension.
  • clang-cl.exe won't compile anonymous struct despite /Ze explicitly stated by users.

@chausner
Copy link

Any chance this could be updated and merged? Once there is CMake support, it should be trivial to publish the library for vcpkg (#112).

@zamazan4ik
Copy link

Any updates on this? @lewissbaker are you interested in providing a way to build and use your library with "default" build-system for C++ now instead of your hand-written stuff?

I cannot use your library only because of custom build system.

P.S. Thank you a lot for the library!

@digimatic
Copy link

Same here for us @zamazan4ik, I use another (old fork) with CMake support for now. But we build for many platforms that this this build system do not support.
CMake is de-facto standard in C++ now..

@luncliff
Copy link

luncliff commented May 28, 2020

Hi. I forgot to mention microsoft/vcpkg#10693 here. I wish you can try with the find_package with it. If you meet some port bug, please let me know.

@dutow
Copy link

dutow commented Jul 18, 2020

Is there a chance of this getting merged in the near future? I've got the library working with GCC 10.1 based on the current master branch and this commit (using cmake, I didn't try to understand how the cake script works), I would open a pull request once this gets merged.

@JohelEGP
Copy link

This looks good. @mmha Remove the draft status so @lewissbaker can merge it.

@dutow dutow mentioned this pull request Sep 9, 2020
@andreasbuhr
Copy link
Contributor

@mmha has never responded after the initial commit here. How can we proceed with this pull request? I'd love to see it merged.

@lewissbaker
Copy link
Owner

Are people generally happy that this PR is good to go?

There were a few comments that hadn't been responded to, yet.

  • Note about /await:heapelide flag for MSVC Release
  • Issue with char8_t on clang-cl (I assume that's a general issue and not specific to cppcoro, though)

Is there a more general CMake solution to turning on coroutines support than
eg. maybe something like in libunifex CMake?
https://github.com/facebookexperimental/libunifex/blob/master/cmake/FindCoroutines.cmake

The other thing that would be good to add soon is CI support, so that we don't regress on the build side of things.

@andreasbuhr
Copy link
Contributor

Hi, thanks for having a look at this pull request.
Garcia6l20 proposed some changes to this pull request in andreasbuhr#1 . I will have a look at them during the weekend and can then create a new pull request here, with Garcia6l20's suggestions included. I will also have a look at the await:heapelide issue.

Concerning the CI support, I started creating some github actions in https://github.com/andreasbuhr/cppcoro/tree/add_github_actions .

Have a look at https://github.com/andreasbuhr/cppcoro/tree/master, README.md, first section. I wrote a little about my initiative to help cppcoro maintenance. If you have any questions, don't hesitate to reach out to me.

@dutow
Copy link

dutow commented Oct 10, 2020

@lewissbaker The libunifex script is longer and more complex, but I don't think it accomplishes more? It certainly isn't more generic, it uses hand specified flags, same as this script. The only addition is that it includes a test C++ source to verify that coroutine compilation works, and it tries to distinguish between final and experimental coroutine support.

For the clang-cl issue, that's not related to this PR. That's one of the possible errors you can get if you use a new version of VS and an old version of clang.

(For this cmake pr: I have ideas on how to improve it, but at least it works, and I can't edit this pull request. Once it's merged, I plan to open another with some improvements)

@andreasbuhr andreasbuhr mentioned this pull request Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants