[tarantool-patches] Re: [PATCH v2] Tarantool static build ability
Georgy Kirichenko
georgy at tarantool.org
Tue Aug 28 23:20:58 MSK 2018
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180828/4e430379/attachment.sig>
More information about the Tarantool-patches
mailing list