This requires a change log request, AFAICS. @ChangeLog - when building tarantool with bundled libcurl, link it with c-ares by default (gh-4591) - add bundled curl and c-ares versions to tarantool version output (gh-4591) -- Serge Petrenko sergepetrenko@tarantool.org > 3 марта 2020 г., в 19:46, Serge Petrenko написал(а): > > 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