Feature/add cmake by ClausKlein · Pull Request #420 · chriskohlhoff/asio

ClausKlein · 2019-08-18T10:07:34Z

this fix #128

I added a .clang-tidy file too. It can be used with cmake while compiling
or as post compile check.

The default compile options are: clang++ -std=c++17 -Wpedantic -Werror

My last compile log is included, see build-with-clang-tidy.log

see too:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
https://www.kdab.com/clang-tidy-part-1-modernize-source-code-using-c11c14/
https://clang.llvm.org/extra/clang-tidy/index.html

The coanan package is created with cmake and tested with ctest

100% tests passed, 0 tests failed out of 124 Total Test time (real) = 60.04 sec

With boost, we can't be pedantic!

prepare .clang-format config fille too

be pedantic by default

fix compile error with socks4 demo; tidy the example CMakeLists.txt; tidy the invocation demo too;

modernize cmake for ccache usage for project asio

…into feature/add_cmake

install asio libray too and export it interface

too change default option: disable handler tracking

The coanan package is created with cmake and tested with ctest

100% tests passed, 0 tests failed out of 124 Total Test time (real) = 60.04 sec

With boost, we can't be pedantic!

prepare .clang-format config fille too

be pedantic by default

modernize cmake for ccache usage for project asio

fix compile error with socks4 demo; tidy the example CMakeLists.txt; tidy the invocation demo too;

install asio libray too and export it interface

too change default option: disable handler tracking

update revision version to 1.14.1 add new cpp17/coroutine example to cmake build

…eature/add_cmake

friendlyanon · 2021-02-27T17:24:06Z

asio/CMakeLists.txt

@@ -0,0 +1,240 @@
+# Minimum supported CMake version
+cmake_minimum_required(VERSION 3.13 FATAL_ERROR)


FATAL_ERROR has been a no-op for the longest time. Please read the docs:
https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html

I will change it to 3.13...3.20

friendlyanon · 2021-02-27T17:24:09Z

asio/CMakeLists.txt

+  # ==========================================================
+
+  include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
+  conan_basic_setup(TARGETS)    # this set CONAN_TARGETS


Use the cmake_find_package* generators. This is intrusive and has no place in the project CML.

friendlyanon · 2021-02-27T17:24:16Z

asio/CMakeLists.txt

+    message(STATUS "use ccache")
+    set(CMAKE_CXX_COMPILER_LAUNCHER "${CCACHE_EXECUTABLE}" CACHE PATH "ccache" FORCE)
+    set(CMAKE_C_COMPILER_LAUNCHER "${CCACHE_EXECUTABLE}" CACHE PATH "ccache" FORCE)
+  endif()


Put this in a superbuild CML, not the project CML. ALternatively, just let people decide whether they want to pass this in from the CLI.

why waste time with compiling?

friendlyanon · 2021-02-27T17:24:22Z

asio/CMakeLists.txt

+  endif()
+else()
+  unset(COMPILER_WARNINGS_ARE_ERRORS CACHE)
+endif()


This has nothing to do in the project CML. Warnings are not a build nor a usage requirement. These should go in the test CML.

friendlyanon · 2021-02-27T17:24:27Z

asio/CMakeLists.txt

+#
+set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/bin)
+set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib)
+set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib)


Why are you forcing a build folder layout? This isn't portable.

this is absolute portable, it is the build tree, not the DESTDIR

friendlyanon · 2021-02-27T17:24:30Z

asio/CMakeLists.txt

+
+
+set(CPPdefinitions)
+set(EXTRA_LIBS)


Everything related to these variables should just go in the respective target_* commands, but wrapped in generator expressions.

friendlyanon · 2021-02-27T17:24:35Z

asio/CMakeLists.txt

+  #######################################################################
+  # use openssl if found
+  set(OPENSSL_ROOT_DIR "/usr" CACHE PATH "path to the openssl lib")
+  find_package(OpenSSL HINTS ${OPENSSL_ROOT_DIR} /opt/local)


Why the hint? You are also missing REQUIRED here.

NO, it is optional!
The HINT was for MACOSX with MacPorts

friendlyanon · 2021-02-27T17:24:39Z

asio/CMakeLists.txt

+    list(APPEND EXTRA_LIBS ${openSSL_LIBRARIES})
+    message(STATUS "HAVE_OPENSSL")
+    list(APPEND CPPdefinitions HAVE_OPENSSL)
+    include_directories(BEFORE ${openSSL_INCLUDE_DIR})


See above. Just use targets.

Yes, this patch is very old.
I will modernise it!

On the other side, yeas passed without a review. What happens now?

friendlyanon · 2021-02-27T17:24:43Z

asio/CMakeLists.txt

+  if(CONAN_LIBS_OPENSSL)
+    list(APPEND CPPdefinitions HAVE_OPENSSL)
+    message(STATUS "have CONAN_LIBS_OPENSSL")
+  endif()


This is just unnecessary noise. The user already knows what's on his system. Why tell him?

friendlyanon · 2021-02-27T17:24:48Z

asio/CMakeLists.txt

+set(ConfigPackageLocation ${CMAKE_INSTALL_LIBDIR}/cmake/asio)
+
+# Interface library
+add_subdirectory(include)


Nitpick, but it's completely unnecessary to break things out like this. I shouldn't have to dive through a bunch of files just to see all the usage and build requirements.

friendlyanon · 2021-02-27T17:24:52Z

asio/CMakeLists.txt

+
+if(ASIO_BUILD_EXAMPLES OR ASIO_BUILD_TESTS)
+  # enable ctest
+  enable_testing()


This is bad, create a separate project CML for tests.

friendlyanon · 2021-02-27T17:24:57Z

asio/CMakeLists.txt

+  DESTINATION ${ConfigPackageLocation})
+install(EXPORT asio-targets
+  DESTINATION ${ConfigPackageLocation}
+  NAMESPACE asio::)


There are many things wrong with these install rules:

Drop configure_package_config_file, you just need a simple config file that you install directly.

You are missing install components. If a user vendors the library then it becomes impossible to tell this library's files apart from his. Since this is a header only library, everything can go in the asio_Development install component.

Where is the include directory installed?

And some nitpicks:

write_basic_package_version_file will pick up the version from the project command and the default output location of the file is already in the current build directory, they just add noise here.

ConfigPackageLocation should be asio_INSTALL_CMAKEDIR, which is a string cache variable marked as advanced. Makes it trivial to relocate the config files for package managers like vcpkg.

friendlyanon · 2021-02-27T17:25:01Z

asio/asio-config.cmake.in

+check_required_components(asio)
+
+include(CMakeFindDependencyMacro)
+find_dependency(OpenSSL QUIET)


This is wrong, you must find dependencies before you make the targets available. The purpose of find_dependency is to forward arguments from the calling find_package command as well. This file should just be:

include(CMakeFindDependencyMacro) find_dependency(OpenSSL) include("${CMAKE_CURRENT_LIST_DIR}/asioTargets.cmake")

See my other comment about htis file not requiring configuration.

friendlyanon · 2021-02-27T17:25:06Z

asio/src/examples/CMakeLists.txt

+  add_subdirectory(cpp03)
+else()
+  message(WARNING "examples/cpp03 needs boost!")
+endif()


This check is unnecessary. Just include the find_package(Boost REQUIRED) call in the cpp03 subproject. If boost is really needed, then the absence of boost is a hard error, not a warning.

Maybe, but asI said cpp03 is historic!

friendlyanon · 2021-02-27T17:25:11Z

asio/src/examples/cpp03/CMakeLists.txt

@@ -0,0 +1,152 @@
+set(target_prefix asio_cpp03_)


This file is not a proper CML, it's missing cmake_minimum_required and project calls.

friendlyanon · 2021-02-27T17:25:18Z

asio/src/examples/cpp03/CMakeLists.txt

+  add_executable(${target_prefix}${target} ${${target}_SOURCES})
+  set_target_properties(${target_prefix}${target} PROPERTIES CXX_STANDARD 98
+    #FIXME CXX_STANDARD 03
+    #cmake error: CXX_STANDARD is set to invalid value '03'


Just use compile features.

friendlyanon · 2021-02-27T17:25:24Z

asio/src/examples/cpp11/fork/daemon.cpp

 #include <asio/ip/udp.hpp>
 #include <asio/signal_set.hpp>
 #include <array>
+#include <cstring>  // strerror used


This PR is about adding CMake. This doesn't look like a CMake related change to me.

There was a time in history, it doesn't compile without this!

friendlyanon · 2021-02-27T17:25:30Z

asio/src/examples/cpp14/CMakeLists.txt

@@ -0,0 +1,43 @@
+set(target_prefix asio_cpp14_)


See cpp03. This is not a proper CML.

friendlyanon · 2021-02-27T17:25:34Z

asio/src/examples/cpp17/CMakeLists.txt

@@ -0,0 +1,33 @@
+set(target_prefix asio_cpp17_)


See cpp03. This is not a proper CML.

friendlyanon · 2021-02-27T17:25:39Z

asio/src/tests/unit/CMakeLists.txt

@@ -0,0 +1,160 @@
+set(target_prefix asio_unit_)


See cpp03. This is not a proper CML.

friendlyanon · 2021-02-27T17:26:27Z

asio/src/tests/unit/CMakeLists.txt

@@ -0,0 +1,160 @@
+set(target_prefix asio_unit_)
+
+add_library(asio::standalone ALIAS standalone)


Why is this aliased here? This should be in the library's project CML, not deeply nested in an incomplete test CML.

friendlyanon · 2021-02-27T17:26:36Z

conan/conanfile.py

@@ -0,0 +1,70 @@
+from conans import ConanFile, CMake, tools


I am not sure about how conan works exactly, but I'm pretty sure this file should be on the CCI instead.

friendlyanon · 2021-02-27T17:26:40Z

conan/conanfile.py

+
+    # NOTE: If header only is used, there is no need for a custom
+    # package_info() method!
+    def package_info(self):


This is not enough to provide the necessary info for cmake_find_package* generators.

Again, yeas ago, conan project started to support boost, I supported asio

friendlyanon

I have reviewed this PR and there are many things that must be improved.

If you want to learn and see how things should be done in CMake, then please take a look at my recent GitHub forks and PRs.

ClausKlein · 2021-02-27T20:08:09Z

I have reviewed this PR and there are many things that must be improved.

If you want to learn and see how things should be done in CMake, then please take a look at my recent GitHub forks and PRs.

If you have time and interest, you may provide a patch for this patch.

Or better, create a new one, simply and clean, use only modern cmake!

And it must work on all OS and with most modern compilers.
c++17 and c++20 only, forget the remaining, forward to future!

And please, merge it than.

ClausKlein · 2021-03-05T13:37:51Z

https://de.wiktionary.org/wiki/gegen_Dummheit_kämpfen_Götter_selbst_vergebens

ClausKlein · 2021-03-05T13:39:40Z

Use this if needed https://github.com/cpm-cmake/CPM.cmake/blob/master/examples/asio-standalone/CMakeLists.txt

ClausKlein and others added 30 commits April 2, 2018 19:56

add conan package setup for standalone version of asio

01f69db

The coanan package is created with cmake and tested with ctest

add conan package setup for standalone version of asio

d978c14

The coanan package is created with cmake and tested with ctest

Merge branch 'develop' of github.com:ClausKlein/asio into develop

e21d574

build header standalon lib with clant-tidy checks

2aec22e

make unit tests compiling again

ea913e1

100% tests passed, 0 tests failed out of 124 Total Test time (real) = 60.04 sec

update examples CMakeLists.txt too

09b2a2e

With boost, we can't be pedantic!

Link pthread lib under linux if standalone

60c2c3d

prepare .clang-format config fille too

fix typo in CMakefile.txt

abb6e6e

be pedantic by default

add missed newer cpp11 examples

56b0b57

fix compile error with socks4 demo; tidy the example CMakeLists.txt; tidy the invocation demo too;

add missing cpp14 examples to cmake build

7aefe2c

cpp17 need compile_options for coroutine

618c061

modernize cmake for ccache usage for project asio

Merge branch 'feature/add_cmake' of https://github.com/ClausKlein/asio …

4148af3

…into feature/add_cmake

use target_link_library if OpenSSL was found

f814b25

install asio libray too and export it interface

export all target compile definitions too

9a2b130

Add missing alias definitons for libs

0f700c7

too change default option: disable handler tracking

add conan package setup for standalone version of asio

c0ec895

The coanan package is created with cmake and tested with ctest

build header standalon lib with clant-tidy checks

82c6499

make unit tests compiling again

a2792d6

100% tests passed, 0 tests failed out of 124 Total Test time (real) = 60.04 sec

Merge branch 'develop' of github.com:ClausKlein/asio into develop

bd07a1f

update examples CMakeLists.txt too

e6f548b

With boost, we can't be pedantic!

Link pthread lib under linux if standalone

66a8c73

prepare .clang-format config fille too

fix typo in CMakefile.txt

a48d06d

be pedantic by default

cpp17 need compile_options for coroutine

7e63a31

modernize cmake for ccache usage for project asio

add missed newer cpp11 examples

e2c775d

fix compile error with socks4 demo; tidy the example CMakeLists.txt; tidy the invocation demo too;

add missing cpp14 examples to cmake build

2864b0f

use target_link_library if OpenSSL was found

b6b6dd3

install asio libray too and export it interface

export all target compile definitions too

5549fb3

Add missing alias definitons for libs

35c4028

too change default option: disable handler tracking

rebase master > develop > feature

6fbf337

update revision version to 1.14.1 add new cpp17/coroutine example to cmake build

Merge branch 'feature/add_cmake' of github.com:ClausKlein/asio into f…

e48687a

…eature/add_cmake

friendlyanon reviewed Feb 27, 2021

View reviewed changes

asio/src/examples/cpp14/CMakeLists.txt

@@ -0,0 +1,43 @@

set(target_prefix asio_cpp14_)

Copy link

friendlyanon Feb 27, 2021

Choose a reason for hiding this comment

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

See cpp03. This is not a proper CML.

friendlyanon reviewed Feb 27, 2021

View reviewed changes

asio/src/examples/cpp17/CMakeLists.txt

@@ -0,0 +1,33 @@

set(target_prefix asio_cpp17_)

Copy link

friendlyanon Feb 27, 2021

Choose a reason for hiding this comment

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

See cpp03. This is not a proper CML.

friendlyanon reviewed Feb 27, 2021

View reviewed changes

asio/src/tests/unit/CMakeLists.txt

@@ -0,0 +1,160 @@

set(target_prefix asio_unit_)

Copy link

friendlyanon Feb 27, 2021

Choose a reason for hiding this comment

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

See cpp03. This is not a proper CML.

friendlyanon reviewed Feb 27, 2021

View reviewed changes

friendlyanon suggested changes Feb 27, 2021

View reviewed changes

ClausKlein marked this pull request as draft February 27, 2021 20:08

ClausKlein closed this Mar 5, 2021

Xaymar mentioned this pull request Jan 18, 2026

Add CMake functionality to export/import cleanly. #1704

Open

		@@ -0,0 +1,240 @@
		# Minimum supported CMake version
		cmake_minimum_required(VERSION 3.13 FATAL_ERROR)

		@@ -0,0 +1,160 @@
		set(target_prefix asio_unit_)

		add_library(asio::standalone ALIAS standalone)



		set(CPPdefinitions)
		set(EXTRA_LIBS)

Conversation

ClausKlein commented Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlyanon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClausKlein commented Feb 27, 2021

Uh oh!

ClausKlein commented Mar 5, 2021

Uh oh!

ClausKlein commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

ClausKlein commented Aug 18, 2019 •

edited

Loading

friendlyanon left a comment •

edited

Loading