cppcoro icon indicating copy to clipboard operation
cppcoro copied to clipboard

GCC 10.1 support

Open dutow opened this issue 3 years ago • 6 comments

This pull requests adds GCC support on top of @mmha's CMake pull request.

I cherry picked his commit to the current master branch for this, I will rebase my pull request once that PR is merged.

I opened this for now only so my changes can be reviewed.

As I explained in my last commit message GCC 10.1 seems to have remaining bugs causing test failures. I found workarounds for some of them, but I had to completely remove one testcase for GCC (not supported syntax), and leave a test failure as is (missing destructor call). Otherwise the library seems to be working.

dutow avatar Jul 19 '20 20:07 dutow

Great job! When can this be merged?

nanoric avatar Aug 14 '20 09:08 nanoric

I've done similar CMake+GCC support for local experiments and implemented several things differently, not quite clean though. Consider replacing for co_await with manual implementation because it hasn't been approved for C++20. Also, why cppcoroConfig.cmake and FindCppcoroCoroutines.cmake are needed there? IIRC, they should be generated outside of project directory where others can reach them. Here's my installation part:

include(GNUInstallDirs)

# install headers
install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../include/cppcoro 
    TYPE INCLUDE
)

# install binary
install(TARGETS cppcoro_lib
    EXPORT coro
    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
    INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

# install config for find_package()
install(EXPORT coro
    DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/cppcoro
    NAMESPACE cppcoro::
    FILE cppcoroConfig.cmake
)

# generate export file
export(EXPORT coro
    NAMESPACE cppcoro::
    FILE cppcoroExport.cmake
)

It's usually good to set default CMAKE_BUILD_TYPE to Release. People then can use canonical CMake installation mkdir build && cd build && cmake .. && cmake --build . && cmake --install .

OleksandrKvl avatar Aug 26 '20 19:08 OleksandrKvl

Just want to say that I'm excited about this.

pbrady avatar Sep 08 '20 20:09 pbrady

@OleksandrKvl the cmake files are from #110, I didn't change those. And I'm not sure if defaulting to release is a good idea, as that would effect multi-buildtype IDE setups also (like visual studio or xcode). If a projects wants to set a default build type correctly, it usually requires several conditions (don't do anything if using a multi config generator ; use debug if the .git directory is present ; use relwithdebinfo otherwise)

@lewissbaker I'll updat my PR based on your comments, but do you have plans about #110? As currently this PR includes that commit too.

dutow avatar Sep 09 '20 06:09 dutow

I've added some comments to #110. Perhaps @OleksandrKvl 's comments above could be added to that PR and CMake-related discussion moved there?

@dutow Would it be possible to separate this PR out into just the code-changes needed for GCC support and not have it build upon the CMake changes, so that they can be merged independently?

lewissbaker avatar Oct 09 '20 20:10 lewissbaker

@lewissbaker Then we don't have a build system I'm actually familiar with. Cake seems to be a custom tool written by you, and the cake script currently forces clang on linux/osx. Seems like adding gcc there would require more changes than the cmake script (adding a config to choose the compiler, then refactoring the entire linux/darwin section to handle both clang and gcc)

Also, then we'll have to add d99be58 to the cmake pullrequest instead.

dutow avatar Oct 10 '20 07:10 dutow