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

[Alexander Turenko <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).

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).