Added CMake build system by mmha · Pull Request #110 · lewissbaker/cppcoro (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

• 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

alexv-ds, santisan, MattEding, zamazan4ik, egor-spk, JohelEGP, fvannee, etorth, and Tadaboody reacted with thumbs up emoji alexv-ds and etorth reacted with hooray emoji denchat, luncliff, alexv-ds, etorth, and Toxe reacted with heart emoji

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.

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.

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.

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🔞
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.

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 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:

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🔞
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.

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

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!

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..

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.

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.

dutow mentioned this pull request

Sep 9, 2020

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

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.

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.

@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)