Hi! Thanks for the review! I fixed your comments and performed the actions you expected me to perform, except one. More info inline. The diff is below. > 8 февр. 2020 г., в 22:42, Alexander Turenko написал(а): > > I mostly okay with the patch, but expect several actions like filing > some issues against c-ares and curl and minor tweaks of the patch. > > Re blocking in threaded resolver > -------------------------------- > > Let's file an issue to curl, because it is inappropriate to block an > application that expect libcurl to give a control if something would > block. An error like 'DNS resolver thread pool is exhausted' is better, > because it can be handled on an application side somehow: > > * do other work / be responsible until free resolver threads will be > available; > * give a user an error for further requests; > * dynamically increase threads count (automatically or upon a user > request). There already exist 2 issues: [1] - closed by a bot, unintentionally, I think. [2] - a duplicate of [1]. > > Re ExternalProject > ------------------ > > In brief: let's file another issue as described below and postpone > further activities. > > There is the way to link c-ares into libcurl w/o installing c-ares using > a pkgconfig file, because it allows to set different include and library > directories and is supported by libcurl's configure script (see [1]). > > This however cannot be used with CMake (I didn't find a way). I propose > to file an issue (or even a PR) to curl to support different directories > for include and library files for c-ares (in CMake build). The > motivation is to use libcurl + c-ares as part of a parent build process > (like we going to do). It seems we just need to add two hints (one for > an include directory and one for a library directory) to > ${CURL}/CMake/FindCARES.cmake (see [2]) like we do in > tarantool/modulekit (see [3]). I’ll think some more about the issue you propose, and file it as soon as I come up with good wording and reasoning. I’m a little bit out of context right now. > > Anyway, I'm okay with ExternalProject() and so installation of c-ares as > a part of build process for now. Hopefully we'll change it in the future > and so we'll not need to adjust flags like -isysroot in our cmake files. > > Re c-ares dependencies > ---------------------- > > See [4]. > > Other > ----- > > BTW, maybe file another issue to lower minimal required CMake version is > c-ares to 2.8? I’ve opened the pull request in c-ares repo [3]. I’ve updated the minimum cmake version to 2.8.12. This is the versrion we tested on and the version that used to be the minimal requirement before it was raised to 3+. > > [1]: https://github.com/curl/curl/issues/2203 > [2]: https://github.com/curl/curl/blob/5ce7102ceae250e2d31b54aad2f33b3bc35f243a/CMake/FindCARES.cmake > [3]: https://github.com/tarantool/modulekit/blob/0994d76d57cc42dd107bd2a9ddcd04ddc91f52da/FindTarantool.cmake#L12 > [4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014024.html > > ---- > >> + set(ARES_CMAKE_FLAGS "-DCARES_STATIC=ON") >> + list(APPEND ARES_CMAKE_FLAGS "-DCARES_SHARED=OFF") >> + list(APPEND ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=OFF") >> + # We buid both static and shared versions of curl, so ares > > Typo: buid. Fixed. > >> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt >> index e12de6005..bdeec5f89 100644 >> --- a/src/CMakeLists.txt >> +++ b/src/CMakeLists.txt >> @@ -227,7 +227,8 @@ if(BUILD_STATIC) >> list(APPEND EXPORT_LIST ${SYMBOLS_LIB}) >> # set variable to allow rescan (CMake depended) >> set(SYMBOLS_LIB "SYMBOLS_LIB-NOTFOUND") >> - elseif (${libstatic} STREQUAL bundled-libcurl) >> + elseif (${libstatic} STREQUAL bundled-libcurl OR >> + ${libstatic} STREQUAL bundled-ares) >> message("We don't need to export symbols from statically linked libcurl, skipped") > > It would be good to adjust the message using ${libstatic} (because not > it is not only about libcurl). Fixed I’ve also fixed your comments from [4], added an empty ARES_LIBRARIES var as a placeholder, and filed an issue to c-ares regarding CMake linking unnecessary libraries [5]. Speaking of linking with unnecessary libraries, as you pointed out in [4], I just left it as-is for now, since everything works in our CI. We may disable the libs altogether later, if it is needed. [1]: https://github.com/curl/curl/issues/2975 [2]: https://github.com/curl/curl/issues/4852 [3]: https://github.com/c-ares/c-ares/pull/306 [4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014024.html [5]: https://github.com/c-ares/c-ares/issues/307 diff --git a/cmake/BuildAres.cmake b/cmake/BuildAres.cmake index 0f9f174ce..a38fa8f30 100644 --- a/cmake/BuildAres.cmake +++ b/cmake/BuildAres.cmake @@ -13,7 +13,7 @@ macro(ares_build) set(ARES_CMAKE_FLAGS "-DCARES_STATIC=ON") list(APPEND ARES_CMAKE_FLAGS "-DCARES_SHARED=OFF") list(APPEND ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=OFF") - # We buid both static and shared versions of curl, so ares + # We build both static and shared versions of curl, so ares # has to be built with -fPIC for the shared version. list(APPEND ARES_CMAKE_FLAGS "-DCARES_STATIC_PIC=ON") # Even though we set the external project's install dir @@ -53,6 +53,7 @@ macro(ares_build) set_target_properties(bundled-ares PROPERTIES IMPORTED_LOCATION ${ARES_INSTALL_DIR}/lib/libcares.a) add_dependencies(bundled-ares bundled-ares-project) + set(ARES_LIBRARIES bundled-ares) unset(ARES_CMAKE_FLAGS) unset(ARES_CFLAGS) diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake index 756297878..5f8b15a63 100644 --- a/cmake/BuildLibCURL.cmake +++ b/cmake/BuildLibCURL.cmake @@ -164,7 +164,7 @@ macro(curl_build) set(CURL_INCLUDE_DIRS ${LIBCURL_INSTALL_DIR}/include) set(CURL_LIBRARIES bundled-libcurl ${LIBZ_LIBRARY}) if (BUNDLED_LIBCURL_USE_ARES) - set(CURL_LIBRARIES ${CURL_LIBRARIES} bundled-ares) + set(CURL_LIBRARIES ${CURL_LIBRARIES} ${ARES_LIBRARIES}) endif() if (TARGET_OS_LINUX OR TARGET_OS_FREEBSD) set(CURL_LIBRARIES ${CURL_LIBRARIES} rt) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index bdeec5f89..7d865472d 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -229,7 +229,7 @@ if(BUILD_STATIC) set(SYMBOLS_LIB "SYMBOLS_LIB-NOTFOUND") elseif (${libstatic} STREQUAL bundled-libcurl OR ${libstatic} STREQUAL bundled-ares) - message("We don't need to export symbols from statically linked libcurl, skipped") + message("We don't need to export symbols from statically linked ${libstatic}, skipped") else() message(WARNING "${libstatic} should be a static") endif() diff --git a/third_party/c-ares b/third_party/c-ares index 56a74c501..bbbffa4da 160000 --- a/third_party/c-ares +++ b/third_party/c-ares @@ -1 +1 @@ -Subproject commit 56a74c501a615c16ca54d608d1796e966f9a503a +Subproject commit bbbffa4da8baf35b0e4e1c376e38018f9a8bcb4e -- Serge Petrenko sergepetrenko@tarantool.org