From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2213C46970E for ; Sat, 8 Feb 2020 22:42:41 +0300 (MSK) Date: Sat, 8 Feb 2020 22:42:55 +0300 From: Alexander Turenko Message-ID: <20200208194255.biakpjlbw3eh3qld@tkn_work_nb> References: <853c32f414c954d38b2621afc7babd57a6e031c4.1579605136.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <853c32f414c954d38b2621afc7babd57a6e031c4.1579605136.git.sergepetrenko@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] build: link bundled libcurl with c-ares List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org 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). 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]). 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? [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. > 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).