Re: [Tarantool-patches] [PATCH v2 1/2] build: link bundled libcurl with c-ares

[Serge Petrenko <sergepetrenko@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8797 bytes --]

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 <sergepetrenko@tarantool.org> написал(а):
> 
> 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 <alexander.turenko@tarantool.org <mailto:alexander.turenko@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).
> 
> 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 <https://github.com/curl/curl/issues/2203>
>> [2]: https://github.com/curl/curl/blob/5ce7102ceae250e2d31b54aad2f33b3bc35f243a/CMake/FindCARES.cmake <https://github.com/curl/curl/blob/5ce7102ceae250e2d31b54aad2f33b3bc35f243a/CMake/FindCARES.cmake>
>> [3]: https://github.com/tarantool/modulekit/blob/0994d76d57cc42dd107bd2a9ddcd04ddc91f52da/FindTarantool.cmake#L12 <https://github.com/tarantool/modulekit/blob/0994d76d57cc42dd107bd2a9ddcd04ddc91f52da/FindTarantool.cmake#L12>
>> [4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014024.html <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 <https://github.com/curl/curl/issues/2975>
> [2]: https://github.com/curl/curl/issues/4852 <https://github.com/curl/curl/issues/4852>
> [3]: https://github.com/c-ares/c-ares/pull/306 <https://github.com/c-ares/c-ares/pull/306>
> [4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014024.html <https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014024.html>
> [5]: https://github.com/c-ares/c-ares/issues/307 <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 <mailto:sergepetrenko@tarantool.org>

[-- Attachment #2: Type: text/html, Size: 16129 bytes --]