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

[Georgy Kirichenko <georgy@tarantool.org>]

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

Hi!

Thanks for the review, i really missed some points from a previous one.
A new patch version was sent, please review.

Some things i would like to add:

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

* it would be good to have a CI for the static build but i think it is out of 
scope of the current issue. Lets create a separate issue for that purpose.

* linking against static open ssl libraries is a requirement from Konstantin 
Nazarov and it is applicable only for Dockerfile included. This Dockerfile used 
only for special enterprise builds. I think it shouldn't be a general rule so 
lets do it while CI integration task.

Regards,
  Georgy Kirichenko

On Sunday, August 26, 2018 5:23:50 AM MSK Alexander Turenko wrote:
> 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/912a6c1cb5949fafe8b17216f16
> > 2a16261d59d0c
> 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.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]