Re: [Tarantool-patches] [PATCH] build: link bundled libcurl with c-ares

[Alexander Turenko <alexander.turenko@tarantool.org>]

Oh, so weird world of build systems. I tried to understand how
everything should work, what is flawed on c-ares build system side and
on our side. This description becomes large, so I'll summarize what
actions I expect from you:

* File an issue against c-ares that CMake script should not add
  unnecessary libraries (./configure does not do that).
* Add empty CARES_LIBRARIES (with proper FIXME comment) and add it to
  CURL_LIBRARIES.

----

> > Is c-ares depends on some libraries?
>
> CMakeLists.txt extract:

Having dependent libraries opens several questions that we should verify
for each of those libs:

1. Is it possible that the library exists on a build system, but may be
   missed on a target system. A build system may be one where we build a
   package (Deb, RPM, brew bottle, FreeBSD binary package), a target
   system is where tarantool is installed from the package.
2. If a library on which our static library (c-ares) depends is not an
   integral part of a system, then we should add it to dependencies of
   packages and link the transitive dependency statically when doing a
   static build of tarantool.

> # Tell C-Ares about libraries to depend on
> IF (HAVE_LIBRESOLV)
> 	LIST (APPEND CARES_DEPENDENT_LIBS resolv)
> ENDIF ()

It is for iPhone targets (see [1]), so we can don't bother with that.

[1]: https://github.com/c-ares/c-ares/commit/affc63cba875df11daade6f6767cb06e013ac6c3

> IF (HAVE_LIBNSL)
> 	LIST (APPEND CARES_DEPENDENT_LIBS nsl)
> ENDIF ()

It just adds -lnsl at linking (there are no ifdefs that depends on this
flag). I guess it is only to borrow gethostbyname() and maybe other
network related functions, which is part of libc.

Points:

* Old glibc ships gethostbyname() in libnsl.so, but now it is right in
  libc.so.
* There is modern separate libnsl (see [2]), which supports IPv6.
* -lnsl is not added by ./configure if gethostbyname() is available w/o
  it.
* gethostbyname() is used only in adig.c and we anyway disable tools with
  -DCARES_BUILD_TOOLS=OFF. Don't sure about other functions from this
  library.

I would work around CMake logic to stick with ./configure logic: add
-lnsl only when gethostbyname() is not availiable w/o it. After this
we'll build c-ares with -lnsl only when libnsl.so is the integral part
of a distro and so both build and install system should have it.
(Update: tarantool executable is linked w/o libnsl.so, so this may be
ignored.)

Let's file an issue to c-ares, which will show different default
behaviour of cmake and configure.

BTW, grpc even disabled this unconditionally, see [3] and [4].

[2]: https://github.com/thkukuk/libnsl
[3]: https://github.com/grpc/grpc/issues/17255
[4]: https://github.com/grpc/grpc/blob/e83426b741d21b744c111109dbac899d6e99b4d7/cmake/cares.cmake#L23

> IF (HAVE_LIBSOCKET)
> 	LIST (APPEND CARES_DEPENDENT_LIBS socket)
> ENDIF ()

It seems it is mostly the same as libnsl: usually don't needed and
should be enable only when necessary and is a part of a system. There
are no however separate library with the same name (I didn't find any)
and so enabling it when it exists looks more or less safe.

So no actions is necessary from our side, however I would anyway mention
in the issue (which I proposed to send above) that it would be better to
enable it only when there are no getaddrinfo() right inside libc.so.

> IF (HAVE_LIBRT)
> 	LIST (APPEND CARES_DEPENDENT_LIBS rt)
> ENDIF ()

c-ares cmake file doing the same bad thing here: it should check whether
clock_gettime() is available in libc.so (w/o extra libs) and only then
check for librt.so. See `man 2 clock_gettime` (cited below) and [5].
Anyway, it should be hurtless in the case, because librt.so either
exists on both build and install systems or doesn't.

From `man 2 clock_gettime`:

 | Link with -lrt (only for glibc versions before 2.17).

[5]: https://github.com/tarantool/tarantool/commit/960e9c0c7ab92155842deaafb40bf240501c8145

----

Since all those libraries are system ones (except new libnsl, but we
should avoid it), static build should be fine: we don't need to add
any static libs at linking of tarantool executable.

It would be good to add all those librt.so, libsocket.so and others to
link command of tarantool executable (and possibly some unit tests) when
they're used to create libcares.a archive. This is what we doing using
CURL_LIBRARIES variable for libcurl. Everything works now, but just
because libraries that are added to libcares.so are not actually needed
on systems we have in CI.

Let's compare c-ares default libs when compiling with the configure
script and CMake on my system (I have the new libnsl installed):

Configure:

 | $ ./buildconf && ./configure && make -j
 | $ ldd .libs/libcares.so
 | 	linux-vdso.so.1 (0x00007ffeaa77b000)
 | 	libc.so.6 => /lib64/libc.so.6 (0x00007fc0ae12c000)
 | 	/lib64/ld-linux-x86-64.so.2 (0x00007fc0ae34a000)

CMake:

 | $ cmake . && make -j
 | $ ldd lib/libcares.so
 | 	linux-vdso.so.1 (0x00007ffceebd6000)
 | 	libnsl.so.2 => /usr/lib64/libnsl.so.2 (0x00007f98321cf000)
 | 	librt.so.1 => /lib64/librt.so.1 (0x00007f98321c5000)
 | 	libc.so.6 => /lib64/libc.so.6 (0x00007f9831ff3000)
 | 	libtirpc.so.3 => /lib64/libtirpc.so.3 (0x00007f9831dc8000)
 | 	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f9831da5000)
 | 	/lib64/ld-linux-x86-64.so.2 (0x00007f983243a000)

NB: I didn't look why libtirpc and libpthread were added. I guess they
are not necessary too.

But tarantool executable does not have, say, libnsl.so.2 and
libtirpc.so.3 in deps (verified on commit
60d67cf38872e012f83410591dfa39140fc679e0):

 | $ ldd src/tarantool
 | 	linux-vdso.so.1 (0x00007ffeb2796000)
 | 	libicui18n.so.65 => /usr/lib64/libicui18n.so.65 (0x00007fe4f3053000)
 | 	libicuuc.so.65 => /usr/lib64/libicuuc.so.65 (0x00007fe4f2e74000)
 | 	libicudata.so.65 => /usr/lib64/libicudata.so.65 (0x00007fe4f13c1000)
 | 	libreadline.so.8 => /lib64/libreadline.so.8 (0x00007fe4f136e000)
 | 	libncurses.so.6 => /lib64/libncurses.so.6 (0x00007fe4f1310000)
 | 	libform.so.6 => /usr/lib64/libform.so.6 (0x00007fe4f12fd000)
 | 	libz.so.1 => /lib64/libz.so.1 (0x00007fe4f12e1000)
 | 	librt.so.1 => /lib64/librt.so.1 (0x00007fe4f12d7000)
 | 	libssl.so.1.1 => /usr/lib64/libssl.so.1.1 (0x00007fe4f126a000)
 | 	libcrypto.so.1.1 => /usr/lib64/libcrypto.so.1.1 (0x00007fe4f0ffd000)
 | 	libdl.so.2 => /lib64/libdl.so.2 (0x00007fe4f0ff7000)
 | 	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fe4f0fd4000)
 | 	libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/libgcc_s.so.1 (0x00007fe4f0fb8000)
 | 	libunwind-x86_64.so.8 => /usr/lib64/libunwind-x86_64.so.8 (0x00007fe4f0f96000)
 | 	libunwind.so.8 => /usr/lib64/libunwind.so.8 (0x00007fe4f0f79000)
 | 	libgomp.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/libgomp.so.1 (0x00007fe4f0f41000)
 | 	libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/libstdc++.so.6 (0x00007fe4f0cc9000)
 | 	libm.so.6 => /lib64/libm.so.6 (0x00007fe4f0b8b000)
 | 	libc.so.6 => /lib64/libc.so.6 (0x00007fe4f09b7000)
 | 	/lib64/ld-linux-x86-64.so.2 (0x00007fe4f39d9000)
 | 	liblzma.so.5 => /lib64/liblzma.so.5 (0x00007fe4f098e000)

So, in short:

* It is good to provide CARES_LIBRARIES, which will be added to
  CURL_LIBRARIES. Even if we'll left it empty for now, it will show
  proper place where to add a library if we'll need to fix a build on
  some specific platform, say, Solaris.
* It would be good to get rid of all unnecessary libraries during c-ares
  build, but since we ignore them anyway at tarantool build and
  everything occasionally works okay on all platforms in CI, then maybe
  we can leave everything as is for now.