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 0061729A1A for ; Tue, 28 Aug 2018 21:25:48 -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 qrCdW5-QuSls for ; Tue, 28 Aug 2018 21:25:47 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 8D602296FF for ; Tue, 28 Aug 2018 21:25:47 -0400 (EDT) Date: Wed, 29 Aug 2018 04:25:47 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v3] Tarantool static build ability Message-ID: <20180829012547.r6nepf4uomq272f2@tkn_work_nb> References: <24642c7850be9d422ea76d96fbcf342bf20f1e5f.1535486630.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <24642c7850be9d422ea76d96fbcf342bf20f1e5f.1535486630.git.georgy@tarantool.org> 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: Georgy Kirichenko Cc: tarantool-patches@freelists.org 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@62614d253850 /]# tarantool PANIC: unprotected error in call to Lua API (builtin/crypto.lua:64: tarantool: undefined symbol: EVP_get_digestbyname)