[Tarantool-patches] [PATCH v2 0/2] build: link curl with c-ares

[Tarantool-patches] [PATCH v2 0/2] build: link curl with c-ares

[Serge Petrenko]

The first commit adds c-ares as a submodule and starts using it when building
libcurl. This allows to eliminate some problems the curl's default threaded
resolver has when DNS server doesn't respond for a long time.

The second commit adds curl and c-ares version output to `tarantool --version`
and also to `require "tarantool"` table in Lua. This may be useful for debugging
purposes.

Since the first commit is intended as a bugfix, it should be pushed to all
branches, starting from 1.10.
However, the second one looks more like a feature so I suggest it be pushed only
to master.

Changes in v2:
  - use CMake when building ares instead of configure
  - minor fixes as per review from @Totktonada

https://github.com/tarantool/tarantool/tree/sp/gh-4591-link-curl-with-c-ares-full-ci
https://github.com/tarantool/tarantool/issues/4591

Serge Petrenko (2):
  build: link bundled libcurl with c-ares
  build: add bundled curl and c-ares to version output

 .gitmodules               |  3 ++
 CMakeLists.txt            |  6 ++++
 cmake/BuildAres.cmake     | 71 +++++++++++++++++++++++++++++++++++++++
 cmake/BuildLibCURL.cmake  | 41 ++++++++++++++++++++--
 src/CMakeLists.txt        |  3 +-
 src/lua/init.c            | 14 ++++++++
 src/main.cc               |  6 ++++
 src/trivia/config.h.cmake |  4 +++
 test/box-py/args.result   |  8 +++++
 test/box-py/args.test.py  |  5 ++-
 third_party/c-ares        |  1 +
 11 files changed, 158 insertions(+), 4 deletions(-)
 create mode 100644 cmake/BuildAres.cmake
 create mode 160000 third_party/c-ares

-- 
2.21.0 (Apple Git-122)

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

[Serge Petrenko]

libcurl has a built-in threaded resolver used for asynchronous DNS
requests, however, when DNS server is slow to respond, the request still
hangs tarantool until it is finished. The reason is that curl calls
thread_join on the resolving thread internally upon timeout, making the
calling thread hang until resolution has ended.
Use c-ares as an asynchronous resolver instead to eliminate the problem.

Closes #4591
---
 .gitmodules              |  3 ++
 CMakeLists.txt           |  6 ++++
 cmake/BuildAres.cmake    | 61 ++++++++++++++++++++++++++++++++++++++++
 cmake/BuildLibCURL.cmake | 31 ++++++++++++++++++--
 src/CMakeLists.txt       |  3 +-
 third_party/c-ares       |  1 +
 6 files changed, 102 insertions(+), 3 deletions(-)
 create mode 100644 cmake/BuildAres.cmake
 create mode 160000 third_party/c-ares

diff --git a/.gitmodules b/.gitmodules
index 76303e0c5..d45e9ce8b 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -40,3 +40,6 @@
 [submodule "third_party/curl"]
 	path = third_party/curl
 	url = https://github.com/tarantool/curl.git
+[submodule "third_party/c-ares"]
+	path = third_party/c-ares
+	url = https://github.com/tarantool/c-ares.git
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ca968eb8d..1d80b6806 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -345,7 +345,13 @@ endif()
 # Curl
 #
 option(ENABLE_BUNDLED_LIBCURL "Enable building of the bundled libcurl" ON)
+option(BUNDLED_LIBCURL_USE_ARES "Build curl with bundled c-ares"
+       ${ENABLE_BUNDLED_LIBCURL})
 if (ENABLE_BUNDLED_LIBCURL)
+    if(BUNDLED_LIBCURL_USE_ARES)
+        include(BuildAres)
+        ares_build()
+    endif()
     include(BuildLibCURL)
     curl_build()
     add_dependencies(build_bundled_libs bundled-libcurl)
diff --git a/cmake/BuildAres.cmake b/cmake/BuildAres.cmake
new file mode 100644
index 000000000..0f9f174ce
--- /dev/null
+++ b/cmake/BuildAres.cmake
@@ -0,0 +1,61 @@
+# A macro to build the bundled libcares
+macro(ares_build)
+    set(ARES_SOURCE_DIR ${PROJECT_SOURCE_DIR}/third_party/c-ares)
+    set(ARES_BINARY_DIR ${PROJECT_BINARY_DIR}/build/ares/work)
+    set(ARES_INSTALL_DIR ${PROJECT_BINARY_DIR}/build/ares/dest)
+
+    # See BuildLibCURL.cmake for details.
+    set(ARES_CFLAGS "")
+    if (TARGET_OS_DARWIN AND NOT "${CMAKE_OSX_SYSROOT}" STREQUAL "")
+        set(ARES_CFLAGS "${ARES_CFLAGS} ${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
+    endif()
+
+    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
+    # 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
+    # below, we still need to pass the corresponding install
+    # prefix via cmake arguments.
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_INSTALL_PREFIX=${ARES_INSTALL_DIR}")
+    # The default values for the options below are not always
+    # "./lib", "./bin"  and "./include", while curl expects them
+    # to be.
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_INSTALL_LIBDIR=lib")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_INSTALL_INCLUDEDIR=include")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_INSTALL_BINDIR=bin")
+
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_LINKER=${CMAKE_LINKER}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_AR=${CMAKE_AR}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_RANLIB=${CMAKE_RANLIB}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_NM=${CMAKE_NM}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_STRIP=${CMAKE_STRIP}")
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_C_FLAGS=${ARES_CFLAGS}")
+    # In hardened mode, which enables -fPIE by default,
+    # the cmake checks don't work without -fPIC.
+    list(APPEND ARES_CMAKE_FLAGS "-DCMAKE_REQUIRED_FLAGS=-fPIC")
+
+    include(ExternalProject)
+    ExternalProject_Add(
+        bundled-ares-project
+        SOURCE_DIR ${ARES_SOURCE_DIR}
+        INSTALL_DIR ${ARES_INSTALL_DIR}
+        DOWNLOAD_DIR ${ARES_BINARY_DIR}
+        TMP_DIR ${ARES_BINARY_DIR}/tmp
+        STAMP_DIR ${ARES_BINARY_DIR}/stamp
+        BINARY_DIR ${ARES_BINARY_DIR}
+        CMAKE_ARGS ${ARES_CMAKE_FLAGS})
+
+    add_library(bundled-ares STATIC IMPORTED GLOBAL)
+    set_target_properties(bundled-ares PROPERTIES IMPORTED_LOCATION
+        ${ARES_INSTALL_DIR}/lib/libcares.a)
+    add_dependencies(bundled-ares bundled-ares-project)
+
+    unset(ARES_CMAKE_FLAGS)
+    unset(ARES_CFLAGS)
+    unset(ARES_BINARY_DIR)
+    unset(ARES_SOURCE_DIR)
+endmacro(ares_build)
diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake
index 6d40ab045..756297878 100644
--- a/cmake/BuildLibCURL.cmake
+++ b/cmake/BuildLibCURL.cmake
@@ -19,6 +19,21 @@ macro(curl_build)
     get_filename_component(FOUND_OPENSSL_ROOT_DIR ${OPENSSL_INCLUDE_DIR} DIRECTORY)
     set(LIBCURL_OPENSSL_OPT "--with-ssl=${FOUND_OPENSSL_ROOT_DIR}")
 
+    # Use either c-ares bundled with tarantool or
+    # libcurl-default threaded resolver.
+    if(BUNDLED_LIBCURL_USE_ARES)
+        set(ASYN_DNS_USED "ares")
+        set(ASYN_DNS_UNUSED "threaded-resolver")
+        set(ASYN_DNS_PATH "=${ARES_INSTALL_DIR}")
+    else()
+        set(ASYN_DNS_USED "threaded-resolver")
+        set(ASYN_DNS_UNUSED "ares")
+        set(ASYN_DNS_PATH "")
+    endif()
+
+    set(ENABLED_DNS_OPT "--enable-${ASYN_DNS_USED}${ASYN_DNS_PATH}")
+    set(DISABLED_DNS_OPT "--disable-${ASYN_DSN_UNUSED}")
+
     # Pass -isysroot=<SDK_PATH> option on Mac OS to a preprocessor
     # and a C compiler to find header files installed with an SDK.
     #
@@ -100,17 +115,17 @@ macro(curl_build)
                 --without-zsh-functions-dir
                 --without-fish-functions-dir
 
+                ${ENABLED_DNS_OPT}
                 --enable-http
                 --enable-proxy
                 --enable-ipv6
-                --enable-threaded-resolver
                 --enable-unix-sockets
                 --enable-cookies
                 --enable-http-auth
                 --enable-mime
                 --enable-dateparse
 
-                --disable-ares
+                ${DISABLED_DNS_OPT}
                 --disable-ftp
                 --disable-file
                 --disable-ldap
@@ -140,14 +155,26 @@ macro(curl_build)
     add_library(bundled-libcurl STATIC IMPORTED GLOBAL)
     set_target_properties(bundled-libcurl PROPERTIES IMPORTED_LOCATION
         ${LIBCURL_INSTALL_DIR}/lib/libcurl.a)
+    if (BUNDLED_LIBCURL_USE_ARES)
+        # Need to build ares first
+        add_dependencies(bundled-libcurl-project bundled-ares)
+    endif()
     add_dependencies(bundled-libcurl bundled-libcurl-project)
 
     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)
+    endif()
     if (TARGET_OS_LINUX OR TARGET_OS_FREEBSD)
         set(CURL_LIBRARIES ${CURL_LIBRARIES} rt)
     endif()
 
+    unset(ASYN_DNS_USED)
+    unset(ASYN_DNS_UNUSED)
+    unset(ASYN_DNS_PATH)
+    unset(ENABLED_DNS_OPT)
+    unset(DISABLED_DNS_OPT)
     unset(LIBCURL_CPPFLAGS)
     unset(LIBCURL_CFLAGS)
     unset(FOUND_OPENSSL_ROOT_DIR)
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")
         else()
             message(WARNING "${libstatic} should be a static")
diff --git a/third_party/c-ares b/third_party/c-ares
new file mode 160000
index 000000000..56a74c501
--- /dev/null
+++ b/third_party/c-ares
@@ -0,0 +1 @@
+Subproject commit 56a74c501a615c16ca54d608d1796e966f9a503a
-- 
2.21.0 (Apple Git-122)

[Tarantool-patches] [PATCH v2 2/2] build: add bundled curl and c-ares to version output

[Serge Petrenko]

Since we may use bundled lubcurl and c-ares, it can be useful to know
the exact curl and c-ares versions tarantool was built with.

Follow-up #4591
---
 cmake/BuildAres.cmake     | 10 ++++++++++
 cmake/BuildLibCURL.cmake  | 10 ++++++++++
 src/lua/init.c            | 14 ++++++++++++++
 src/main.cc               |  6 ++++++
 src/trivia/config.h.cmake |  4 ++++
 test/box-py/args.result   |  8 ++++++++
 test/box-py/args.test.py  |  5 ++++-
 7 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/cmake/BuildAres.cmake b/cmake/BuildAres.cmake
index 0f9f174ce..7be6f5b00 100644
--- a/cmake/BuildAres.cmake
+++ b/cmake/BuildAres.cmake
@@ -4,6 +4,16 @@ macro(ares_build)
     set(ARES_BINARY_DIR ${PROJECT_BINARY_DIR}/build/ares/work)
     set(ARES_INSTALL_DIR ${PROJECT_BINARY_DIR}/build/ares/dest)
 
+    # Get bundled ares version for `tarantool -v` output.
+    if (EXISTS "${ARES_SOURCE_DIR}/.git" AND GIT)
+        execute_process (COMMAND ${GIT} describe HEAD
+            OUTPUT_VARIABLE BUNDLED_ARES_VERSION
+            OUTPUT_STRIP_TRAILING_WHITESPACE
+            WORKING_DIRECTORY ${ARES_SOURCE_DIR})
+            string(REPLACE cares- "" BUNDLED_ARES_VERSION ${BUNDLED_ARES_VERSION})
+            string(REPLACE _ . BUNDLED_ARES_VERSION ${BUNDLED_ARES_VERSION})
+    endif()
+
     # See BuildLibCURL.cmake for details.
     set(ARES_CFLAGS "")
     if (TARGET_OS_DARWIN AND NOT "${CMAKE_OSX_SYSROOT}" STREQUAL "")
diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake
index 756297878..aff34cbf0 100644
--- a/cmake/BuildLibCURL.cmake
+++ b/cmake/BuildLibCURL.cmake
@@ -4,6 +4,16 @@ macro(curl_build)
     set(LIBCURL_BINARY_DIR ${PROJECT_BINARY_DIR}/build/curl/work)
     set(LIBCURL_INSTALL_DIR ${PROJECT_BINARY_DIR}/build/curl/dest)
 
+    # Get bundled curl version for `tarantool -v` output.
+    if (EXISTS "${LIBCURL_SOURCE_DIR}/.git" AND GIT)
+        execute_process (COMMAND ${GIT} describe HEAD
+            OUTPUT_VARIABLE BUNDLED_CURL_VERSION
+            OUTPUT_STRIP_TRAILING_WHITESPACE
+            WORKING_DIRECTORY ${LIBCURL_SOURCE_DIR})
+            string(REPLACE curl- "" BUNDLED_CURL_VERSION ${BUNDLED_CURL_VERSION})
+            string(REPLACE _ . BUNDLED_CURL_VERSION ${BUNDLED_CURL_VERSION})
+    endif()
+
     if (BUILD_STATIC)
         set(LIBZ_LIB_NAME libz.a)
     else()
diff --git a/src/lua/init.c b/src/lua/init.c
index 097dd8495..437175260 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -427,6 +427,20 @@ luaopen_tarantool(lua_State *L)
 	lua_pushstring(L, TARANTOOL_C_FLAGS);
 	lua_settable(L, -3);
 
+#ifdef BUNDLED_CURL_VERSION
+	/* build.curl */
+	lua_pushstring(L, "curl");
+	lua_pushstring(L, BUNDLED_CURL_VERSION);
+	lua_settable(L, -3);
+#endif
+
+#ifdef BUNDLED_ARES_VERSION
+	/* build.c_ares */
+	lua_pushstring(L, "c_ares");
+	lua_pushstring(L, BUNDLED_ARES_VERSION);
+	lua_settable(L, -3);
+#endif
+
 	lua_settable(L, -3);    /* box.info.build */
 	return 1;
 }
diff --git a/src/main.cc b/src/main.cc
index e674d85b1..098b97013 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -675,6 +675,12 @@ print_version(void)
 	printf("%s %s\n", tarantool_package(), tarantool_version());
 	printf("Target: %s\n", BUILD_INFO);
 	printf("Build options: %s\n", BUILD_OPTIONS);
+#ifdef BUNDLED_CURL_VERSION
+	printf("Bundled libCURL version: %s\n", BUNDLED_CURL_VERSION);
+#endif
+#ifdef BUNDLED_ARES_VERSION
+	printf("Bundled c-ares version: %s\n", BUNDLED_ARES_VERSION);
+#endif
 	printf("Compiler: %s\n", COMPILER_INFO);
 	printf("C_FLAGS:%s\n", TARANTOOL_C_FLAGS);
 	printf("CXX_FLAGS:%s\n", TARANTOOL_CXX_FLAGS);
diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
index 226f83128..c541beb31 100644
--- a/src/trivia/config.h.cmake
+++ b/src/trivia/config.h.cmake
@@ -42,6 +42,10 @@
 #define TARANTOOL_LIBEXT "so"
 #endif
 
+/** Is set to the version of builtin libcurl used. */
+#cmakedefine BUNDLED_CURL_VERSION "${BUNDLED_CURL_VERSION}"
+/** Is set to the version of builtin c-ares used. */
+#cmakedefine BUNDLED_ARES_VERSION "${BUNDLED_ARES_VERSION}"
 /**
  * Defined if cpuid() instruction is available.
  */
diff --git a/test/box-py/args.result b/test/box-py/args.result
index 54629edea..cb1131eb3 100644
--- a/test/box-py/args.result
+++ b/test/box-py/args.result
@@ -47,6 +47,8 @@ tarantool --version
 Tarantool 2.minor.patch-<rev>-<commit>
 Target: platform <build>
 Build options: flags
+Curl 7.66.patch
+c-ares 1.15.patch-<rev>-<commit>
 Compiler: cc
 C_FLAGS: flags
 CXX_FLAGS: flags
@@ -55,6 +57,8 @@ tarantool -v
 Tarantool 2.minor.patch-<rev>-<commit>
 Target: platform <build>
 Build options: flags
+Curl 7.66.patch
+c-ares 1.15.patch-<rev>-<commit>
 Compiler: cc
 C_FLAGS: flags
 CXX_FLAGS: flags
@@ -63,6 +67,8 @@ tarantool -V
 Tarantool 2.minor.patch-<rev>-<commit>
 Target: platform <build>
 Build options: flags
+Curl 7.66.patch
+c-ares 1.15.patch-<rev>-<commit>
 Compiler: cc
 C_FLAGS: flags
 CXX_FLAGS: flags
@@ -114,6 +120,8 @@ tarantool -V ${SOURCEDIR}/test/box-py/args.lua 1 2 3
 Tarantool 2.minor.patch-<rev>-<commit>
 Target: platform <build>
 Build options: flags
+Curl 7.66.patch
+c-ares 1.15.patch-<rev>-<commit>
 Compiler: cc
 C_FLAGS: flags
 CXX_FLAGS: flags
diff --git a/test/box-py/args.test.py b/test/box-py/args.test.py
index f89c5bb09..59b0f4b4c 100644
--- a/test/box-py/args.test.py
+++ b/test/box-py/args.test.py
@@ -16,7 +16,10 @@ sys.stdout.push_filter("unrecognized option.*", "unrecognized option")
 server.test_option("-Z")
 server.test_option("--no-such-option")
 server.test_option("--no-such-option --version")
-sys.stdout.push_filter(".* (\d+)\.\d+\.\d(-\d+-\w+)?", "Tarantool \\1.minor.patch-<rev>-<commit>")
+sys.stdout.push_filter("Tarantool (\d+)\.\d+\.\d(-\d+-\w+)?", "Tarantool \\1.minor.patch-<rev>-<commit>")
+sys.stdout.push_filter("Bundled libCURL version: (\d+).(\d+)\.\d+", "Curl \\1.\\2.patch")
+sys.stdout.push_filter("Bundled c-ares version: (\d+)\.(\d+)\.\d(-\d+-\w+)?",
+                       "c-ares \\1.\\2.patch-<rev>-<commit>")
 sys.stdout.push_filter("Target: .*", "Target: platform <build>")
 sys.stdout.push_filter(".*Disable shared arena since.*\n", "")
 sys.stdout.push_filter("Build options: .*", "Build options: flags")
-- 
2.21.0 (Apple Git-122)

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

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

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

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

[Serge Petrenko]

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

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> написал(а):
> 
> 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 <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


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

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

[Serge Petrenko]

[-- 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 --]

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

[Alexander Turenko]

> > 8 февр. 2020 г., в 22:42, 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).
> 
> There already exist 2 issues: [1] - closed by a bot, unintentionally, I think.
> [2] - a duplicate of [1]. 

Thanks! Answered to the issue.

> [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>

Re: [Tarantool-patches] [PATCH v2 2/2] build: add bundled curl and c-ares to version output

[Alexander Turenko]

On Tue, Jan 21, 2020 at 02:54:32PM +0300, Serge Petrenko wrote:
> Since we may use bundled lubcurl and c-ares, it can be useful to know
> the exact curl and c-ares versions tarantool was built with.
> 
> Follow-up #4591
> ---
>  cmake/BuildAres.cmake     | 10 ++++++++++
>  cmake/BuildLibCURL.cmake  | 10 ++++++++++
>  src/lua/init.c            | 14 ++++++++++++++
>  src/main.cc               |  6 ++++++
>  src/trivia/config.h.cmake |  4 ++++
>  test/box-py/args.result   |  8 ++++++++
>  test/box-py/args.test.py  |  5 ++++-
>  7 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/cmake/BuildAres.cmake b/cmake/BuildAres.cmake
> index 0f9f174ce..7be6f5b00 100644
> --- a/cmake/BuildAres.cmake
> +++ b/cmake/BuildAres.cmake
> @@ -4,6 +4,16 @@ macro(ares_build)
>      set(ARES_BINARY_DIR ${PROJECT_BINARY_DIR}/build/ares/work)
>      set(ARES_INSTALL_DIR ${PROJECT_BINARY_DIR}/build/ares/dest)
>  
> +    # Get bundled ares version for `tarantool -v` output.
> +    if (EXISTS "${ARES_SOURCE_DIR}/.git" AND GIT)
> +        execute_process (COMMAND ${GIT} describe HEAD
> +            OUTPUT_VARIABLE BUNDLED_ARES_VERSION
> +            OUTPUT_STRIP_TRAILING_WHITESPACE
> +            WORKING_DIRECTORY ${ARES_SOURCE_DIR})
> +            string(REPLACE cares- "" BUNDLED_ARES_VERSION ${BUNDLED_ARES_VERSION})
> +            string(REPLACE _ . BUNDLED_ARES_VERSION ${BUNDLED_ARES_VERSION})
> +    endif()

This way does not work when tarantool sources are fetched from a tarball
(rather from git repository). It'll affect all packages, our ones and
external. Our packages are built from tarballs, created by packpack
before run rpmbuild / debuild. External packages (like Gentoo) also
fetch tarballs, so it will not work too. Likely the same on Mac OS
(brew) and FreeBSD (port / pkg).

For tarantool itself it is handles using a VERSION file, which is alway
shipped within a tarball. packpack stores a git version in this file
when creating a tarball. CMakeLists.txt reads it if exists.

How to check:

$ TARBALL_COMPRESSOR=gz ~/p/t/packpack/packpack tarball

It'll create a tarball in build directory. You can unpack it and build.

The reason why *-full-ci does not fail is that box-py test suite is not
included yet in all testing jobs (see [1]).

To be honest, I don't see a way to overcome this problem. Need to think
more. Maybe add some hooks to packpack to create extra VERSION files.

[1]: https://github.com/tarantool/tarantool/issues/4599

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

[Alexander Turenko]

On Tue, Mar 03, 2020 at 07:46:48PM +0300, Serge Petrenko wrote:
> 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.

This patch LGTM. CCed Kirill.

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

[Kirill Yukhin]

Hello,

On 21 янв 14:54, Serge Petrenko wrote:
> libcurl has a built-in threaded resolver used for asynchronous DNS
> requests, however, when DNS server is slow to respond, the request still
> hangs tarantool until it is finished. The reason is that curl calls
> thread_join on the resolving thread internally upon timeout, making the
> calling thread hang until resolution has ended.
> Use c-ares as an asynchronous resolver instead to eliminate the problem.
> 
> Closes #4591

I've checked your patch into 1.10, 2.2, 2.3 and master.

--
Regards, Kirill Yukhin