<HTML><BODY>Alexander, thanks for the review, I've made all changes as you suggested, please recheck the new version of the patch. Also I've divided the patch into 2 patch sets and left under the issue #4551 only changes for static build w/o Dockerfile.<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 5 декабря 2019, 17:55 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY">I take a very brief glance.<br><br>
First, I think this patch should be splitted to several ones: fixes in<br>
cmake files for static build (one for libunwind, maybe another re libdl<br>
(didn't get this change)), downloading of tarballs in the docker file,<br>
the docker file clean up, parallel testing in the docker file, new CI<br>
jobs.<br></div></div></div></div></blockquote>Right, the patch divided into standalone patch set with Dockerfile<br> changes and patch for static build w/o it.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
See more comments below.<br><br>
Please, look first to the comment marked as (*) -- this email contains<br>
points that depend on it.<br><br>
WBR, Alexander Turenko.<br><br>
On Wed, Nov 27, 2019 at 03:33:30PM +0300, Alexander V. Tikhonov wrote:<br>
> Fixed static build with '-DBUILD_STATIC=ON' option:<br>
> - added lzma library installation to Dockerfile for static build<br><br>
I don't see a reason for this change except that liblzma was added to<br>
required libraries (under -DBUILD_STATIC=ON), which is not right thing<br>
to do IMHO (see (*)).<br><br>
I guess the change is harmless, but it is not motivated so should not be<br>
pushed.<br></div></div></div></div></blockquote>Right, found that after your suggestions it is not needed at all - removed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> - added lzma library to unwind library for Tarantool build at file:<br>
> cmake/compiler.cmake<br><br>
Commented below (see (*)).</div></div></div></div></blockquote><p>Right, fixed as you suggested and added comments that the library will<br>be used if it will be found.</p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br><br>
> - added dl library to gomp and icu libraries for test/unit tests<br>
> binaries builds at files:<br>
> cmake/BuildMisc.cmake<br>
> cmake/FindICU.cmake<br><br>
A reason here is not clear. It does not work before? In some<br>
environment? With some options?<br></div></div></div></div></blockquote>Building unit tests got linking errors - all of them posted at the commit<br>message.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> Added static build to gitlab-ci in release check criteria named<br>
> as static_build job. Previously named static_build job renamed to<br>
> static_docker_build, due to it checks the build at Dockerfile.<br><br>
It worth to move it to its own commit.</div></div></div></div></blockquote>It seems not good to add the died code w/o its use, seems that better<br>to use single commit for it.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br><br>
> <br>
> Made the following changes at Dockerfile for static build:<br>
> - changed 'wget' tool use to 'curl -O -L' to avoid of '500' HTTP<br>
> error respond from download servers<br><br>
As we discussed before, the problem was not in the wget/curl, but in a<br>
tarballs hosting. You are fixed in this the next item, so there are no<br>
reasons for this change. Please, revert.<br></div></div></div></div></blockquote>I've made a lot checks and found that the curl tool use instead of wget<br>helped to resolve flaky issues, also I've added the links to the issues at<br>the sourceforge site to see how it happened worldwide.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> - changed the link from sourceforge to github to download<br>
> the icu4c sources, as suggested on icu4c web site<br><br>
It is not obvious why the change was made from this message. Let's add<br>
'because of flaky connection failures' or kinda (I don't know exact<br>
problem).</div></div></div></div></blockquote>Ok, I've completely rewrote the comment.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br><br>
> - added patch tool to Docker image bootstrap to be able to patch the<br>
> lzma sources<br><br>
Should gone with removing lzma hard dependency (see (*)).<br></div></div></div></div></blockquote>Right, it is not needed now - removed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> - added 'build' directory removement to avoid of old configuration<br>
> at build/curl which is used for curl building<br>
> - removed LD_LIBRARY_PATH environment from curl build, due to the<br>
> path is empty in real and is not needed<br>
> - added '-j' option to make tool calls for all builds to speed it up<br>
> in 4 times<br>
> - set Dockerfile WORKDIR from the very start of Tarantool sources<br>
> builds to make the Dockerfile code more readable and removed all<br>
> duplicating calls to Tarantool sources directory changes.<br><br>
Let's move all cleanup changes into its own commit. Let's move speed up<br>
to its own commit too.</div></div></div></div></blockquote>Created standalone patch set with 5 commits in it for each of the change.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br><br>
> <br>
> Close #4551<br>
> ---<br>
> <br>
> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/gh-4551-static-build-full-ci" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/gh-4551-static-build-full-ci</a><br>
> Issue: <a href="https://github.com/tarantool/tarantool/issues/4551" target="_blank">https://github.com/tarantool/tarantool/issues/4551</a><br>
> <br>
> .gitlab-ci.yml | 12 ++++++++++-<br>
> .gitlab.mk | 7 +++++-<br>
> Dockerfile.staticbuild | 48 ++++++++++++++++++++++++------------------<br>
> cmake/BuildMisc.cmake | 2 +-<br>
> cmake/FindICU.cmake | 2 +-<br>
> cmake/compiler.cmake | 9 ++++++++<br>
> 6 files changed, 56 insertions(+), 24 deletions(-)<br>
> <br>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml<br>
> index cf13c382e..3bdcfa745 100644<br>
> --- a/.gitlab-ci.yml<br>
> +++ b/.gitlab-ci.yml<br>
> @@ -230,9 +230,19 @@ debian_10:<br>
> OS: 'debian'<br>
> DIST: 'buster'<br>
> <br>
> +# Static builds<br>
> +<br>
> static_build:<br>
> + <<: *release_only_definition<br>
> + <<: *docker_test_definition<br>
> + variables:<br>
> + CMAKE_EXTRA_PARAMS: '-DBUILD_STATIC=ON'<br>
> + script:<br>
> + - ${GITLAB_MAKE} static_build<br>
> +<br>
> +static_docker_build:<br>
> <<: *deploy_test_definition<br>
> variables:<br>
> RUN_TESTS: 'ON'<br>
> script:<br>
> - - ${GITLAB_MAKE} static_build<br>
> + - ${GITLAB_MAKE} static_docker_build<br>
> diff --git a/.gitlab.mk b/.gitlab.mk<br>
> index 48a92e518..a5d4ec26e 100644<br>
> --- a/.gitlab.mk<br>
> +++ b/.gitlab.mk<br>
> @@ -110,5 +110,10 @@ package: git_submodule_update<br>
> # Static build<br>
> # ############<br>
> <br>
> -static_build:<br>
> +deps_debian_static:<br>
> + apt install -y -f liblzma-dev<br><br>
We use apt-get everywhere in .travis.mk, let's use it here too for<br>
consistency.</div></div></div></div></blockquote>Ok, also tried to avoid of it, and found that on Debian Stretch OS the<br>libunwind static library has undefined use of liblzma functions, because<br> there is no liblzma static library. Found that libunwind dynamic uses<br>existing liblzma dynamic library while, and that is why we need to install<br> liblzma-dev package to additionally provide liblzma static library.<div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br><br>
> +<br>
> +static_build: deps_debian_static test_debian_no_deps<br><br>
'test_debian_no_deps' depends on 'build_debian', which doing cmake with<br>
${CMAKE_EXTRA_PARAMS}. Nothing about static build here. Why this make<br>
goal is named static_build?<br><br>
Yes, I know, CMAKE_EXTRA_PARAMS is set in a gitlab job and there are no<br>
questions about gitlab job name.<br><br>
Let's remove 'static_build' and 'deps_debian_static' make goals and use<br>
just 'test_debian_no_deps' for 'static_build' gitlab job (with custom<br>
CMAKE_EXTRA_PARAMS)?<br></div></div></div></div></blockquote>Ok, as was discussed by voice decided to move static build make targets from<br> .gitlab.mk to .travis.mk to store it in common place with the other test/build make<br> targets. Moved environment from .gitlab-ci.yml file into make targets to<p> make this targets true building in static w/o additional setup.</p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> +<br>
> +static_docker_build:<br>
> docker build --network=host --build-arg RUN_TESTS="${RUN_TESTS}" -f Dockerfile.staticbuild .<br>
> diff --git a/Dockerfile.staticbuild b/Dockerfile.staticbuild<br>
> index d63c18bd1..8ca003262 100644<br>
> --- a/Dockerfile.staticbuild<br>
> +++ b/Dockerfile.staticbuild<br>
> @@ -20,6 +20,7 @@ RUN set -x \<br>
> unzip \<br>
> libunwind \<br>
> zlib \<br>
> + patch \<br>
> && yum -y install \<br>
> perl \<br>
> gcc-c++ \<br>
> @@ -44,50 +45,57 @@ RUN set -x && \<br>
> tar -xvf openssl-1.1.0h.tar.gz && \<br>
> cd openssl-1.1.0h && \<br>
> ./config --libdir=lib && \<br>
> - make && make install<br>
> + make -j && make install<br>
> <br>
> RUN set -x && \<br>
> cd / && \<br>
> - wget <a href="http://download.icu-project.org/files/icu4c/62.1/icu4c-62_1-src.tgz" target="_blank">http://download.icu-project.org/files/icu4c/62.1/icu4c-62_1-src.tgz</a> && \<br>
> + curl -O -L <a href="https://github.com/unicode-org/icu/releases/download/release-62-1/icu4c-62_1-src.tgz" target="_blank">https://github.com/unicode-org/icu/releases/download/release-62-1/icu4c-62_1-src.tgz</a> && \<br>
> tar -xvf icu4c-62_1-src.tgz && \<br>
> cd icu/source && \<br>
> ./configure --with-data-packaging=static --enable-static --enable-shared && \<br>
> - make && make install<br>
> + make -j && make install<br>
> <br>
> RUN set -x && \<br>
> cd / && \<br>
> - LD_LIBRARY_PATH=/usr/local/lib64 curl -O -L <a href="http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz" target="_blank">http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz</a> && \<br>
> + curl -O -L <a href="http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz" target="_blank">http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz</a> && \<br>
> tar -xvf libunwind-1.3-rc1.tar.gz && \<br>
> cd libunwind-1.3-rc1 && \<br>
> ./configure --enable-static --enable-shared && \<br>
> - make && make install<br>
> + make -j && make install<br>
> +<br>
> +RUN set -x && \<br>
> + mkdir /liblzma && cd /liblzma && \<br>
> + yumdownloader --source xz-devel && \<br>
> + rpm2cpio xz-*.src.rpm | cpio -idv && \<br>
> + tar xf xz-*.tar.gz && \<br>
> + for dir in * ; do cd ${dir} 2>/dev/null && break ; done && \<br>
> + for patch in ../*.patch ; do patch -Np1 < ${patch} ; done && \<br>
> + ./configure --enable-static && \<br>
> + make -j && make install<br>
> <br>
> COPY . /tarantool<br>
> <br>
> +WORKDIR /tarantool<br>
> +<br>
> RUN set -x && \<br>
> - cd tarantool && \<br>
> git submodule init && \<br>
> git submodule update<br>
> <br>
> -WORKDIR /tarantool<br>
> -<br>
> RUN set -x && \<br>
> find . -name 'CMakeFiles' -type d -exec rm -rf {} + && \<br>
> - find . -name 'CMakeCache.txt' -type f -delete<br>
> + find . -name 'CMakeCache.txt' -type f -delete && \<br>
> + rm -rf build<br>
> <br>
> RUN pip install -r /tarantool/test-run/requirements.txt<br>
> <br>
> -RUN set -x \<br>
> - && (cd /tarantool; \<br>
> - cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \<br>
> - -DENABLE_DIST:BOOL=ON \<br>
> - -DBUILD_STATIC=ON \<br>
> - -DOPENSSL_USE_STATIC_LIBS=ON \<br>
> - -DOPENSSL_ROOT_DIR=/usr/local \<br>
> - .) \<br>
> - && make -C /tarantool -j<br>
> -<br>
> -RUN cd /tarantool && make install<br>
> +RUN set -x && \<br>
> + cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \<br>
> + -DENABLE_DIST:BOOL=ON \<br>
> + -DBUILD_STATIC=ON \<br>
> + -DOPENSSL_USE_STATIC_LIBS=ON \<br>
> + -DOPENSSL_ROOT_DIR=/usr/local \<br>
> + . && \<br>
> + make -j && make install<br>
> <br>
> ARG RUN_TESTS<br>
> RUN if [ -n "${RUN_TESTS}" ]; then \<br>
> diff --git a/cmake/BuildMisc.cmake b/cmake/BuildMisc.cmake<br>
> index b4a3ca1fc..0c571f326 100644<br>
> --- a/cmake/BuildMisc.cmake<br>
> +++ b/cmake/BuildMisc.cmake<br>
> @@ -39,7 +39,7 @@ macro(libmisc_build)<br>
> else()<br>
> set(GOMP_LIBRARY gomp)<br>
> endif()<br>
> - target_link_libraries(misc ${GOMP_LIBRARY} pthread)<br>
> + target_link_libraries(misc ${GOMP_LIBRARY} pthread dl)<br>
> endif()<br>
> <br>
> unset(misc_src)<br>
> diff --git a/cmake/FindICU.cmake b/cmake/FindICU.cmake<br>
> index 26f0683f3..2bb9a5d59 100644<br>
> --- a/cmake/FindICU.cmake<br>
> +++ b/cmake/FindICU.cmake<br>
> @@ -50,7 +50,7 @@ include(FindPackageHandleStandardArgs)<br>
> find_package_handle_standard_args(ICU<br>
> REQUIRED_VARS ICU_INCLUDE_DIR ICU_LIBRARY_I18N ICU_LIBRARY_UC)<br>
> set(ICU_INCLUDE_DIRS ${ICU_INCLUDE_DIR})<br>
> -set(ICU_LIBRARIES ${ICU_LIBRARY_I18N} ${ICU_LIBRARY_UC} ${ICU_LIBRARY_DATA})<br>
> +set(ICU_LIBRARIES ${ICU_LIBRARY_I18N} ${ICU_LIBRARY_UC} ${ICU_LIBRARY_DATA} dl)<br>
> mark_as_advanced(ICU_INCLUDE_DIR ICU_INCLUDE_DIRS<br>
> ICU_LIBRARY_I18N ICU_LIBRARY_UC ICU_LIBRARIES)<br>
> <br>
> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake<br>
> index c9ad2b092..00fac1c66 100644<br>
> --- a/cmake/compiler.cmake<br>
> +++ b/cmake/compiler.cmake<br>
> @@ -171,6 +171,15 @@ if (ENABLE_BACKTRACE)<br>
> NAMES ${UNWIND_PLATFORM_LIB_NAME})<br>
> set(UNWIND_LIBRARIES ${UNWIND_PLATFORM_LIBRARY} ${UNWIND_LIBRARY})<br>
> endif()<br>
> + if(BUILD_STATIC)<br>
> + set(LZMA_LIB_NAME liblzma.a)<br>
> + find_library(LZMA_LIBRARY PATH_SUFFIXES system NAMES ${LZMA_LIB_NAME})<br>
> + if (NOT LZMA_LIBRARY)<br>
> + message (FATAL_ERROR "ENABLE_BACKTRACE option is set but lzma "<br>
> + "library is not found")<br><br>
(*)<br><br>
ENABLE_BACKTRACE does not directly requires liblzma, but only when<br>
libunwind requires it.<br><br>
Moreover, we don't require lzma anywhere else and, say, does not depend<br>
on it in packages.<br><br>
What behaviour is desired here: if libunwind requires liblzma (it is<br>
build-time option), then we should always add -llzma after -lunwind (I<br>
assume both after -static), since the latter requires symbols from the<br>
former.<br><br>
It is nor clear (at least for me) how to determine whether one static<br>
library depends on another in a general case. The approach we use over<br>
cmake files is a kind of workaround: if a library is installed on<br>
default paths, then we add it, because it is harmless even if, say,<br>
libunwind does not depends on liblzma.<br><br>
Let's adopt this way here and don't give an error if liblzma is not<br>
found.<br><br>
If you have a vision how to handle transitive dependencies for static<br>
libraries in a better way, let's share it (say, file an issue).<br></div></div></div></div></blockquote>Ok, I've made changes based on your suggestions.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15755577250259942079_BODY"><br>
> + endif()<br>
> + set(UNWIND_LIBRARIES ${UNWIND_LIBRARIES} ${LZMA_LIBRARY})<br>
> + endif()<br>
> find_package_message(UNWIND_LIBRARIES "Found unwind" "${UNWIND_LIBRARIES}")<br>
> endif()<br>
> <br>
> -- <br>
> 2.17.1<br>
> <br></div></div></div></div></blockquote>
<br>
<br>-- <br>Alexander Tikhonov<br></BODY></HTML>