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

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

Hi!

Below I updated the discussions raised in the first review, add one
thoughts what is worth to do, add results of experimenting with the new
build option and added inline comments to the code (mostly style
nitpicking).

The main things are: prevent building of a dynamic executabe with
-DBUILD_STATIC=ON and add CI target to test it.

WBR, Alexander Turenko.

---- Updates ----

I'll cite mine comments for the previous version of the patch below
(except obvious and fixed ones) and will state what was decided (and
what was missed) on the later verbal discussion with Georgy.

1.

> Can we wrap find_package to some macro that will pay attention to
> BUILD_STATIC value. Even if not all cases will be covered with this
> approach, it seems that most of them will do.

a) Georgy against autoconverting of suffixes (.so -> .a), because it
   can lead to unobvious problems. I agree.

b) A list of libraries can be different for dynamic and static build, because
   we must list dependencies of statically linked libraries. But that is about
   different find_package() calls and unrelated here.

I think it would be better to pass list of dynamic library names and
static library names separatelly to a wrapper function where test
BUILD_STATIC. Like so:

find_library_ext(Z_LIBRARY
    NAMES_DYNAMIC z
    NAMES_STATIC libz.a)

Googled and found that such wrapper would not be straighforward to
implement (will involve cmake_parse_arguments and so). So (as least on
the mine level of cmake knowledge) it worth to leave simple
implementation as we have. The question is closed.

2.

> Why do you need to change order of find_package? If there is some
> specific order we should follow (say, because of curl depends on
> openssl) it would be good to mention that in the commit message. And I
> hope there is more proper way to handle dependencies in cmake (instead
> of ordering).

I googled around myself and don't found simple approach. So the question can be
considered as closed.

3.

> (re cmake/FindOpenSSL.cmake)
> Note: it is not the last version of the file. The commit [2] was made
> since that one.
> [2]: https://gitlab.kitware.com/cmake/cmake/commit/912a6c1cb5949fafe8b17216f162a16261d59d0c

We catched file from the commit made 9 months ago and don't catch one
from 4 month ago. Both versions are released now. Don't understand a
criteria here. The question remains open.

4.

> (re crc32 -> tnt_crc32 and so)
> Some conflict that are not mentioned in the commit message?

Was not answered and was not added to the commit message. The question
remains open.

---- New thoughts ----

We definitely should enable the new target in CI (at least check
building).

---- Experimenting ----

I tried to compile tarantool w/o static libraries installed and got the
error that libz.a was not found (due to CMAKE_REQUIRED_LIBRARIES). This
is right behaviour.

Then I installed static libz and the build was 'successful': it produces
dynamic executable. Some warnings was shown:

> CMake Warning at src/CMakeLists.txt:288 (message):
>   /usr/lib/libcurses.so should be a static
>   /usr/lib/libform.so should be a static
>   /usr/lib/libcurl.so should be a static
>   /usr/lib64/libssl.so should be a static
>   /usr/lib64/libcrypto.so should be a static
>   /usr/lib/libicui18n.so should be a static
>   /usr/lib/libicuuc.so should be a static
>   yaml should be a static
>
> ...
>
> [  5%] Generating ../extra/exports.Linux
> SYMBOLS_LIB-NOTFOUND /usr/lib/libicudata.so /usr/lib/libz.so
> nm: 'SYMBOLS_LIB-NOTFOUND': No such file
> cat: SYMBOLS_LIB-NOTFOUND: No such file or directory
> nm: 'a.out': No such file

I think is is error-prone to allow to produce dynamic executable with
-DBUILD_STATIC=ON. The behaviour is such, because find_library contains both
names: libxxx.a and xxx. When libxxx.a is not found xxx.so will be used.

I propose the following approach instead:

--- a/cmake/FindICU.cmake
+++ b/cmake/FindICU.cmake
@@ -23,21 +23,25 @@ find_path(ICU_INCLUDE_DIR
 )

 if(BUILD_STATIC)
-    set(ICU_I18N_STATIC libicui18n.a)
-    set(ICU_UC_STATIC libicuuc.a)
-    set(ICU_DATA_STATIC libicudata.a)
+    set(ICU_I18N_LIBRARY_NAME libicui18n.a)
+    set(ICU_UC_LIBRARY_NAME libicuuc.a)
+    set(ICU_DATA_LIBRARY_NAME libicudata.a)
+else()
+    set(ICU_I18N_LIBRARY_NAME icui18n)
+    set(ICU_UC_LIBRARY_NAME icuuc)
+    set(ICU_DATA_LIBRARY_NAME icudata)
 endif()

-find_library(ICU_LIBRARY_I18N NAMES ${ICU_I18N_STATIC} icui18n
+find_library(ICU_LIBRARY_I18N NAMES ${ICU_I18N_LIBRARY_NAME}
     HINTS ${ICU_FIND_LIBRARY_HINTS}
     ${ICU_FIND_OPTS}
 )
-find_library(ICU_LIBRARY_UC NAMES ${ICU_UC_STATIC} icuuc
+find_library(ICU_LIBRARY_UC NAMES ${ICU_UC_LIBRARY_NAME}
     HINTS ${ICU_FIND_LIBRARY_HINTS}
     ${ICU_FIND_OPTS}
 )

-find_library(ICU_LIBRARY_DATA NAMES ${ICU_DATA_STATIC} icudata
+find_library(ICU_LIBRARY_DATA NAMES ${ICU_DATA_LIBRARY_NAME}
     HINTS ${ICU_FIND_LIBRARY_HINTS}
     ${ICU_FIND_OPTS}
 )

---- Inline comments ----

On Wed, Aug 22, 2018 at 05:25:55PM +0300, Georgy Kirichenko wrote:
> A possibility to build tarantool with included library dependencies.
> Use the flag -DBUILD_STATIC=ON to build statically against curl, readline,
> ncurses, icu and z.
> Use the flag -DOPENSSL_USE_STATIC_LIBS=ON to build with static
> openssl
> 
> Changes:
>   * Add FindOpenSSL.cmake because some distributions do not support the use of
>   openssl static libraries.
>   * Find libssl before curl because of build dependency.
>   * Catch all bundled libraries API and export then it in case of static
>   build.
> 
> Notes:
>   * Bundled libyaml is not properly exported, use the system one.
>   * Dockerfile to build static with docker is included
> 
> Fixes #3445
> ---
> Changes in v2:
>   - Fixed comments as per review by Alexander Turenko
> Issue: https://github.com/tarantool/tarantool/issues/3445
> Branch:
> https://github.com/tarantool/tarantool/tree/g.kirichenko/static-build
> 

> diff --git a/Dockerfile.staticbuild b/Dockerfile.staticbuild
> new file mode 100644
> index 000000000..8e44dd13c
> --- /dev/null
> +++ b/Dockerfile.staticbuild
> @@ -0,0 +1,100 @@
> +FROM centos:7
> +
> +RUN yum install -y epel-release
> +RUN yum install -y yum install https://centos7.iuscommunity.org/ius-release.rpm
> +
> +RUN set -x \
> +    && yum -y install \
> +        libstdc++ \
> +        libstdc++-static \
> +        readline \
> +        openssl \
> +        lz4 \
> +        binutils \
> +        ncurses \
> +        libgomp \
> +        lua \
> +        curl \
> +        tar \
> +        zip \
> +        unzip \
> +        libunwind \
> +        libcurl \
> +    && yum -y install \
> +        perl \
> +        gcc-c++ \
> +        cmake \
> +        lz4-devel \
> +        binutils-devel \
> +        lua-devel \
> +        make \
> +        git \
> +        autoconf \
> +        automake \
> +        libtool \
> +        wget
> +
> +

Two newlines instead of one.

> +RUN yum -y install ncurses-static readline-static zlib-static pcre-static glibc-static
> +
> +RUN set -x && \
> +    cd / && \
> +    curl -O -L https://www.openssl.org/source/openssl-1.1.0h.tar.gz && \
> +    tar -xvf openssl-1.1.0h.tar.gz && \
> +    cd openssl-1.1.0h && \
> +    ./config no-shared && \
> +    make && make install
> +
> +RUN set -x && \
> +    cd / && \
> +    git clone https://github.com/curl/curl.git && \
> +    cd curl && \
> +    git checkout curl-7_59_0 && \
> +    ./buildconf && \
> +    LIBS=" -lssl -lcrypto -ldl" ./configure --enable-static --enable-shared --with-ssl && \
> +    make -j && make install
> +
> +RUN set -x &&\

Backslash w/o a space before it.

> +RUN set -x \
> +    && (cd /tarantool; \
> +       cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo\
> +             -DENABLE_BUNDLED_LIBYAML:BOOL=OFF\
> +             -DENABLE_DIST:BOOL=ON\
> +	     -DBUILD_STATIC=ON\
> +	     -DOPENSSL_USE_STATIC_LIBS=ON\
> +             .) \
> +    && make -C /tarantool -j
> +

1. Two lines has tabs instead of whitespaces.

2. Backslashes do not separated from a previous word by a whitespace.

3. Are you sure we should provide statically bundled OpenSSL by default?
   I think it is the bad idea. Static build is for environments with
   strong politics about security, so inability to update actually used
   openssl version w/o software vendor can be issue.

> diff --git a/cmake/BuildMisc.cmake b/cmake/BuildMisc.cmake
> index 20ecb4f63..5b09ed5c2 100644
> --- a/cmake/BuildMisc.cmake
> +++ b/cmake/BuildMisc.cmake
> @@ -33,5 +33,16 @@ 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()
> +
> +

Two newlines instead on one.

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

Is this should be under if(BUILD_STATIC)?

> diff --git a/extra/mkexports b/extra/mkexports
> index 20b3454d4..2f39c5012 100755
> --- a/extra/mkexports
> +++ b/extra/mkexports
> @@ -2,6 +2,7 @@
>  # $1 - in  file
>  # $2 - out file
>  # $3 - os
> +# $4 - export templates
>  if [ "x$3x" = xDarwinx ]; then
>      # _func1
>      # _func2
> @@ -11,5 +12,16 @@ else
>      #   func1;
>      #   func2;
>      # };
> -    ( echo "{" && sed -e '/^\s*$/d;s/$/;/;' $1 && echo "};" ) > $2
> +    echo "$4"
> +    ( echo "{" && {
> +      # combine static defined list of functions
> +      cat $1 ; 
> +      # with list of exported functions of bundled libraries
> +      for so in $4 ; do {
> +        # exported names from shared object
> +        nm -D $so ||
> +        # or follow patch from shared object script
> +        nm -D `cat $so | grep GROUP | awk '{print $3}'` ; 
> +      } | awk '{print $3}' ; done ;
> +    } | sed '/^\s*$/d;s/$/;/;' && echo "};" ) > $2
>  fi

Trailing whitespaces.

> +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}
> +            ${LIBYAML_LIBRARIES})
> +        if (${libstatic} MATCHES "lib[^/]+.a")
> +            string(REGEX MATCH "lib[^/]+.a" libname ${libstatic})
> +            string(REGEX REPLACE "\\.a$" "" libname ${libname})
> +            string(REGEX REPLACE "^lib" "" libname ${libname})
> +            find_library(SYMBOLS_LIB NAMES ${libname})
> +	    # add found library to export list
> +            list(APPEND EXPORT_LIST ${SYMBOLS_LIB})
> +	    # set variable to allow rescan (CMake depended)

Tabs instead of whitespaces before comments.