to perform, except one. More info inline. The diff is below.
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+.
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