[tarantool-patches] Re: [PATCH v3] Tarantool static build ability

[Alexander Turenko]

Hi, Georgy!

I have minor notes on the code and some experimenting results. Please,
especially consider attempt to use docker at end of the email.

I don't think I can add more value on the next review rounds, so,
please, proceed with the next reviewer (I guess Kirill Yu.) if the
process allows it.

WBR, Alexander Turenko.

---- Updates ----

>From answers to comments to v2:

> * there is no possibility to use a new version of FindOpenSSL.cmake module
> because line 411 of the module:
> `include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake)'
> requires for other file in our internal cmake directory.

Included cmake file has the same line at the line 374 (fixed in the our repo,
of cource, see the first review, where I show the diff).

I don't insist on including the last version and don't sure whether it would
better from, say, portability reasons. I'm okay with the version included now,
just was curious. Let's consider the question closed.

---- Inline comments ----

> --- a/cmake/BuildMisc.cmake
> +++ b/cmake/BuildMisc.cmake
> @@ -33,5 +33,15 @@ macro(libmisc_build)
>  
>      add_library(misc STATIC ${misc_src})
>  
> +    if (HAVE_OPENMP)
> +        target_compile_options(misc PRIVATE "-fopenmp")
> +        if(BUILD_STATIC)
> +            set(GOMP_LIBRARY libgomp.a)
> +        else()
> +            set(GOMP_LIBRARY gomp)
> +        endif()
> +        target_link_libraries(misc pthread ${GOMP_LIBRARY})
> +    endif()
> +
>      unset(misc_src)
>  endmacro(libmisc_build)

Are not pthread should follows gomp? nm reports undefined static symbols starts
with 'pthread_' in libgomp.a.

>  if(CURL_FOUND)
>    set(CURL_LIBRARIES ${CURL_LIBRARY})
>    set(CURL_INCLUDE_DIRS ${CURL_INCLUDE_DIR})
> -  set(CMAKE_REQUIRED_LIBRARIES ${CURL_LIBRARIES})
> +  set(CMAKE_REQUIRED_LIBRARIES ${CURL_LIBRARIES} ${OPENSSL_LIBRARIES} ${Z_LIBRARY} pthread dl)

libcurl.a can be linked with other libraries (nghttp2 at least) and we
possibly should take care of it to perform static build successfully on
different OSes. It is not important in context of the issue you work on,
but maybe will matter in the future. So I just note it here and don't
know whether we should do something later.

> diff --git a/cmake/FindReadline.cmake b/cmake/FindReadline.cmake
> index 681a6f5de..770c4e15f 100644
> --- a/cmake/FindReadline.cmake
> +++ b/cmake/FindReadline.cmake
> @@ -6,22 +6,37 @@
>  # READLINE_LIBRARIES
>  #
>  
> +if(BUILD_STATIC)
> +    find_library(CURSES_CURSES_LIBRARY NAMES libcurses.a)
> +    find_library(CURSES_NCURSES_LIBRARY NAMES libncurses.a)
> +    find_library(CURSES_FORM_LIBRARY NAMES libform.a)
> +    find_library(CURSES_INFO_LIBRARY NAMES libtinfo.a)
> +    if (NOT CURSES_INFO_LIBRARY)
> +        set(CURSES_INFO_LIBRARY "")
> +    endif()
> +endif()

CURSES_CURSES_LIBRARY is not used.
CURSES_NCURSES_LIBRARY is not used.
CURSES_FORM_LIBRARY is not used.

I don't get the point of this block (except placing libtinfo.a into deps if it
is present). Please, clarify it or remove the dead code.

> +if(BUILD_STATIC)
> +    # Static linking for c++ routines
> +    add_compile_flags("C;CXX" "-static-libgcc" "-static-libstdc++")
> +endif()
> +

Maybe it should also be under an option, because static linking with a
libc can make the resulting executable less portable (I guess because of
compatibility with new kernels). Security updates questiong is arisen
here too. I think most of user don't want it and it should be disable by
default.

Sorry, I miss this before.

> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 2a952923e..e9b6458ef 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -107,18 +107,19 @@ endif ()
>  
>  add_library(core STATIC ${core_sources})
>  target_link_libraries(core
> -    salad small pthread
> +    salad small
>      ${LIBEV_LIBRARIES}
>      ${LIBEIO_LIBRARIES}
>      ${LIBCORO_LIBRARIES}
>      ${MSGPUCK_LIBRARIES}
> +    dl pthread
>  )
>  

You are use `generic_libraries` variable for pthread and dl below. Maybe
set the variable above and use it here too?

> +if(BUILD_STATIC)
> +    set(Z_STATIC libz.a)
> +endif()
> +find_library(Z_LIBRARY NAMES ${Z_STATIC} z)
> +

Note: Z_LIBRARY is not used below.

Do we want to add libz dependency *only* in case of static build? If so,
I think we should write it like so:

if(BUILD_STATIC)
    find_library(Z_LIBRARY_STATIC NAMES libz.a)
endif()

And use Z_LIBRARY_STATIC below instead of Z_STATIC.

If I understand it wrongly and we okay to add libz dependency in case of
dynamic build we can just use ${Z_LIBRARY} below, because we already set it in
cmake/FindCURL.cmake. It seems the case is similar to libgomp one, but we don't
avoid linking the dynamic version for gomp (however we could).

>  set (common_libraries
>      ${reexport_libraries}
>      ${LIBYAML_LIBRARIES}
>      ${READLINE_LIBRARIES}
> -    ${OPENSSL_LIBRARIES}
>      ${CURL_LIBRARIES}
> +    ${OPENSSL_LIBRARIES}
> +    ${Z_STATIC}
>  )
>  

This hunk cannot be applied automatically on top of the current 1.10,
because of adding ${ICONV_LIBRARIES} in dcac64af. Can you please rebase
on top of 1.10? The rest are applied successfully.

Note: nghttp2 is needed here too if libcurl.a needs it (see the comment
above). (No changes are required, just note.)

> +set(EXPORT_LIST)
> +if(BUILD_STATIC)
> +    # for each static library we should find a corresponding shared library to
> +    # parse and reexport library api functions
> +    foreach(libstatic
> +            ${READLINE_LIBRARIES}
> +            ${CURL_LIBRARIES}
> +            ${OPENSSL_LIBRARIES}
> +            ${ICU_LIBRARIES}
> +            ${Z_STATIC}

Note: don't forget to change Z_STATIC here to reflect changes above if
you'll do it.

> @@ -295,6 +333,7 @@ if (TARGET_OS_DARWIN)
>          LINK_FLAGS "-Wl,-exported_symbols_list,${exports_file}")
>  else ()
>      target_link_libraries(tarantool
> +                          ${generic_libraries}
>                            -Wl,--whole-archive box ${reexport_libraries}
>                            salad -Wl,--no-whole-archive
>                            ${common_libraries})

You have ${generic_libraries} added at end of core.a target, but here it is at
the start. I think generic libraries should follows others in general to
collect all needed symbols before resolving with the generic libraries.

If you place it at start to avoid link as static, don't afraid. It seems
that cmake correctly juggles with -Wl,-Bstatic and -Wl,-Bdynamic to get
static libs static and dynamic libs dynamic.

---- Experimenting (take 2) ----

Environment: Gentoo; /etc/portage/package.use/tarantool contains:

sys-libs/zlib static-libs
net-misc/curl static-libs
dev-libs/openssl static-libs
sys-libs/readline static-libs
sys-libs/ncurses static-libs
dev-libs/icu static-libs

net-misc/curl USE flags: http2 ipv6 ssl static-libs ABI_X86="64"
CURL_SSL="openssl" (the comment about nghttp2 is because of the http2
flag here).

Have added nghttp2 in two cmake files as noted above.

I'm able to build tarantool.

The following warnings I got on the configure stage:

CMake Warning at src/CMakeLists.txt:285 (message):
  /usr/lib64/libssl.so should be a static
  /usr/lib64/libcrypto.so should be a static
  yaml should be a static

The following warnings I got on the build stage:

nm: /usr/lib/libz.so: File format not recognized
nm: /usr/lib/libcurses.so: File format not recognized
nm: /usr/lib/libz.so: File format not recognized
(maybe missed some of them)

$ lddtree src/tarantool 
tarantool => src/tarantool (interpreter => /lib64/ld-linux-x86-64.so.2)
    libpthread.so.0 => /lib64/libpthread.so.0
    libdl.so.2 => /lib64/libdl.so.2
    libssl.so.1.0.0 => /usr/lib64/libssl.so.1.0.0
    libcrypto.so.1.0.0 => /usr/lib64/libcrypto.so.1.0.0
        libz.so.1 => /lib64/libz.so.1
    libnghttp2.so.14 => /usr/lib64/libnghttp2.so.14
    librt.so.1 => /lib64/librt.so.1
    libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libgcc_s.so.1
    libm.so.6 => /lib64/libm.so.6
    libc.so.6 => /lib64/libc.so.6
    ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2

The executable runs, the interactive session looks a bit strange (but
ok):

tarantool> require('strict')
tarantool> ---
- on: 'function: 0x40046128'
  off: 'function: 0x40046108'
...

Compare with mine system tarantool:

tarantool> require('strict')
---
- on: 'function: 0x412ff1b0'
  off: 'function: 0x412ff190'

Tried with -DOPENSSL_USE_STATIC_LIBS=ON.

At configure:

CMake Warning at src/CMakeLists.txt:286 (message):
  yaml should be a static

At build:

/usr/lib/libreadline.so /usr/lib/libcurses.so /usr/lib/libform.so /usr/lib/libcurl.so /usr/lib/libssl.so /usr/lib/libcrypto.so /usr/lib/libicui18n.so /usr/lib/libicuuc.so /usr/lib/libicudata.so /usr/lib/libz.so (some debug are forgotten?)
nm: /usr/lib/libz.so: File format not recognized
nm: /usr/lib/libreadline.so: File format not recognized
(maybe missed some of them)

$ lddtree src/tarantool 
tarantool => src/tarantool (interpreter => /lib64/ld-linux-x86-64.so.2)
    libpthread.so.0 => /lib64/libpthread.so.0
    libdl.so.2 => /lib64/libdl.so.2
    libnghttp2.so.14 => /usr/lib64/libnghttp2.so.14
    librt.so.1 => /lib64/librt.so.1
    libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libgcc_s.so.1
    libm.so.6 => /lib64/libm.so.6
    libc.so.6 => /lib64/libc.so.6
    ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2

Don't sure why libgcc_s is here, but I think it is okay.

Runs okay (the same as the previous one).

Dynamic build is okay too:

$ lddtree src/tarantool 
tarantool => src/tarantool (interpreter => /lib64/ld-linux-x86-64.so.2)
    libicui18n.so.60 => /usr/lib64/libicui18n.so.60
    libicuuc.so.60 => /usr/lib64/libicuuc.so.60
        libicudata.so.60 => /usr/lib64/libicudata.so.60
    libreadline.so.7 => /lib64/libreadline.so.7
    libncurses.so.6 => /lib64/libncurses.so.6
    libform.so.6 => /usr/lib64/libform.so.6
    libssl.so.1.0.0 => /usr/lib64/libssl.so.1.0.0
    libcrypto.so.1.0.0 => /usr/lib64/libcrypto.so.1.0.0
        libz.so.1 => /lib64/libz.so.1
    libcurl.so.4 => /usr/lib64/libcurl.so.4
        libnghttp2.so.14 => /usr/lib64/libnghttp2.so.14
    libdl.so.2 => /lib64/libdl.so.2
        ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2
    librt.so.1 => /lib64/librt.so.1
    libpthread.so.0 => /lib64/libpthread.so.0
    libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libgcc_s.so.1
    libunwind.so.8 => /usr/lib64/libunwind.so.8
    libunwind-x86_64.so.8 => /usr/lib64/libunwind-x86_64.so.8
    libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libstdc++.so.6
    libm.so.6 => /lib64/libm.so.6
    libgomp.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libgomp.so.1
    libc.so.6 => /lib64/libc.so.6

Runs okay.

Trying docker:

$ docker build -t staticbuild -f Dockerfile.staticbuild .
$ docker run -it staticbuild
[root at 62614d253850 /]# tarantool
PANIC: unprotected error in call to Lua API (builtin/crypto.lua:64: tarantool: undefined symbol: EVP_get_digestbyname)