From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 49D8D29568 for ; Tue, 28 Aug 2018 16:21:05 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7sC9T7O-JRcP for ; Tue, 28 Aug 2018 16:21:05 -0400 (EDT) Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D787229267 for ; Tue, 28 Aug 2018 16:21:04 -0400 (EDT) From: Georgy Kirichenko Subject: [tarantool-patches] Re: [PATCH v2] Tarantool static build ability Date: Tue, 28 Aug 2018 23:20:58 +0300 Message-ID: <2083471.OCNYbPLJSz@home.lan> In-Reply-To: <20180826022350.2neahfr4r27icssz@tkn_work_nb> References: <82a00b0d7cb943f54b9b63e44e9205e8247d177a.1534947866.git.georgy@tarantool.org> <20180826022350.2neahfr4r27icssz@tkn_work_nb> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2111361.eczAFfThNn"; micalg="pgp-sha256"; protocol="application/pgp-signature" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko Cc: tarantool-patches@freelists.org --nextPart2111361.eczAFfThNn Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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. --nextPart2111361.eczAFfThNn Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEBJFDbU76LsBbgHBsvKOmCX79zb4FAluFrqoACgkQvKOmCX79 zb5aTgf/bJAYbjOQivElC51G8O51yRHdxmTvP4i1k3o6nEBP2dyLgDMIv3A7IXRB 0e8grHH+2jb0Pf7fVj7QeGpwu/wmei+qlZWv7i5di3O1BuqCe7SFt/YZzlFgdZ0I 4gC69PMQ8lyiQOFnXdpORuOJGPvCY+YY8ktTZeXhXB6MKBAut2XOHbn8HIzNxuQN VXc9jYgFkJobDoMstYdXt+bsjP0IGqPeCHnrDGGUbUf76O9yDZ7Iz1aqESdGnFzN WNgvxyKjDP5MOYbcnexStNWh5mo6T2DAdeU/H5I159uyYB1LJZwxlv3gqN1o4rnX 9wGF2g7cT0RWPXc8aeXM7gM3bylJpQ== =NwND -----END PGP SIGNATURE----- --nextPart2111361.eczAFfThNn--