From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 396DA469719 for ; Tue, 3 Mar 2020 19:51:06 +0300 (MSK) From: Serge Petrenko Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_03FFD070-1A7A-4232-827A-DD55D0BE3F35" Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\)) Date: Tue, 3 Mar 2020 19:51:05 +0300 In-Reply-To: <94B97C48-7273-418B-B404-CC714348254A@tarantool.org> References: <853c32f414c954d38b2621afc7babd57a6e031c4.1579605136.git.sergepetrenko@tarantool.org> <20200208194255.biakpjlbw3eh3qld@tkn_work_nb> <94B97C48-7273-418B-B404-CC714348254A@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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org --Apple-Mail=_03FFD070-1A7A-4232-827A-DD55D0BE3F35 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 19:46, Serge = Petrenko =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > 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. >=20 >> 8 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 22:42, Alexander = Turenko > =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0): >>=20 >> 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. >>=20 >> Re blocking in threaded resolver >> -------------------------------- >>=20 >> 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: >>=20 >> * 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). >=20 > There already exist 2 issues: [1] - closed by a bot, unintentionally, = I think. > [2] - a duplicate of [1].=20 >=20 >>=20 >> Re ExternalProject >> ------------------ >>=20 >> In brief: let's file another issue as described below and postpone >> further activities. >>=20 >> 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]). >>=20 >> 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]). >=20 > I=E2=80=99ll think some more about the issue you propose, and file it = as soon as I > come up with good wording and reasoning. I=E2=80=99m a little bit out = of context > right now. >=20 >>=20 >> 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. >>=20 >> Re c-ares dependencies >> ---------------------- >>=20 >> See [4]. >>=20 >> Other >> ----- >>=20 >> BTW, maybe file another issue to lower minimal required CMake version = is >> c-ares to 2.8? >=20 > I=E2=80=99ve opened the pull request in c-ares repo [3]. > I=E2=80=99ve 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+. >=20 >>=20 >> [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/0994d76d57cc42dd107bd2a9ddcd04= ddc91f52da/FindTarantool.cmake#L12 = >> [4]: = https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/0140= 24.html = >>=20 >> ---- >>=20 >>> + set(ARES_CMAKE_FLAGS "-DCARES_STATIC=3DON") >>> + list(APPEND ARES_CMAKE_FLAGS "-DCARES_SHARED=3DOFF") >>> + list(APPEND ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=3DOFF") >>> + # We buid both static and shared versions of curl, so ares >>=20 >> Typo: buid. >=20 > Fixed. >=20 >>=20 >>> 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") >>=20 >> It would be good to adjust the message using ${libstatic} (because = not >> it is not only about libcurl). >=20 > Fixed >=20 > I=E2=80=99ve 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]. >=20 > 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. >=20 > [1]: https://github.com/curl/curl/issues/2975 = > [2]: https://github.com/curl/curl/issues/4852 = > [3]: https://github.com/c-ares/c-ares/pull/306 = > [4]: = https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/0140= 24.html = > [5]: https://github.com/c-ares/c-ares/issues/307 = >=20 > 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=3DON") > list(APPEND ARES_CMAKE_FLAGS "-DCARES_SHARED=3DOFF") > list(APPEND ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=3DOFF") > - # 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=3DON") > # 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) > =20 > 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 >=20 > -- > Serge Petrenko > sergepetrenko@tarantool.org --Apple-Mail=_03FFD070-1A7A-4232-827A-DD55D0BE3F35 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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)




3= =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 19:46, Serge = Petrenko <sergepetrenko@tarantool.org> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

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 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2= 22:42, Alexander Turenko <alexander.turenko@tarantool.org> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

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=E2=80=99ll think some = more about the issue you propose, and file it as soon as I
come up with good wording and reasoning. I=E2=80=99m 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=E2=80=99ve opened the pull request in = c-ares repo [3].
I=E2=80=99ve 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/5ce7102ceae250e2d31b54aad2f33= b3bc35f243a/CMake/FindCARES.cmake
[3]: https://github.com/tarantool/modulekit/blob/0994d76d57cc42dd107= bd2a9ddcd04ddc91f52da/FindTarantool.cmake#L12
[4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-Fe= bruary/014024.html

----
+ =    set(ARES_CMAKE_FLAGS "-DCARES_STATIC=3DON")
+    list(APPEND ARES_CMAKE_FLAGS = "-DCARES_SHARED=3DOFF")
+    list(APPEND = ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=3DOFF")
+ =    # 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)
          &nb= sp; list(APPEND EXPORT_LIST ${SYMBOLS_LIB})
          &nb= sp; # set variable to allow rescan (CMake depended)
          &nb= sp; set(SYMBOLS_LIB "SYMBOLS_LIB-NOTFOUND")
- =        elseif (${libstatic} STREQUAL = bundled-libcurl)
+ =        elseif (${libstatic} STREQUAL = bundled-libcurl OR
+ =             &n= bsp;  ${libstatic} STREQUAL bundled-ares)
          &nb= sp; 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=E2=80=99ve 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
[2]: https://github.com/curl/curl/issues/4852
[3]: https://github.com/c-ares/c-ares/pull/306
[4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-Fe= bruary/014024.html
[5]: 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=3DON")
     list(APPEND = ARES_CMAKE_FLAGS "-DCARES_SHARED=3DOFF")
    =  list(APPEND ARES_CMAKE_FLAGS "-DCARES_BUILD_TOOLS=3DOFF")
-    # 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=3DON")
     # 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
=
= --Apple-Mail=_03FFD070-1A7A-4232-827A-DD55D0BE3F35--