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.


Четверг, 5 декабря 2019, 17:55 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:

I take a very brief glance.

First, I think this patch should be splitted to several ones: fixes in
cmake files for static build (one for libunwind, maybe another re libdl
(didn't get this change)), downloading of tarballs in the docker file,
the docker file clean up, parallel testing in the docker file, new CI
jobs.
Right, the patch divided into standalone patch set with Dockerfile
changes and patch for static build w/o it.

See more comments below.

Please, look first to the comment marked as (*) -- this email contains
points that depend on it.

WBR, Alexander Turenko.

On Wed, Nov 27, 2019 at 03:33:30PM +0300, Alexander V. Tikhonov wrote:
> Fixed static build with '-DBUILD_STATIC=ON' option:
> - added lzma library installation to Dockerfile for static build

I don't see a reason for this change except that liblzma was added to
required libraries (under -DBUILD_STATIC=ON), which is not right thing
to do IMHO (see (*)).

I guess the change is harmless, but it is not motivated so should not be
pushed.
Right, found that after your suggestions it is not needed at all - removed.

> - added lzma library to unwind library for Tarantool build at file:
> cmake/compiler.cmake

Commented below (see (*)).

Right, fixed as you suggested and added comments that the library will
be used if it will be found.



> - added dl library to gomp and icu libraries for test/unit tests
> binaries builds at files:
> cmake/BuildMisc.cmake
> cmake/FindICU.cmake

A reason here is not clear. It does not work before? In some
environment? With some options?
Building unit tests got linking errors - all of them posted at the commit
message.

> Added static build to gitlab-ci in release check criteria named
> as static_build job. Previously named static_build job renamed to
> static_docker_build, due to it checks the build at Dockerfile.

It worth to move it to its own commit.
It seems not good to add the died code w/o its use, seems that better
to use single commit for it.


>
> Made the following changes at Dockerfile for static build:
> - changed 'wget' tool use to 'curl -O -L' to avoid of '500' HTTP
> error respond from download servers

As we discussed before, the problem was not in the wget/curl, but in a
tarballs hosting. You are fixed in this the next item, so there are no
reasons for this change. Please, revert.
I've made a lot checks and found that the curl tool use instead of wget
helped to resolve flaky issues, also I've added the links to the issues at
the sourceforge site to see how it happened worldwide.

> - changed the link from sourceforge to github to download
> the icu4c sources, as suggested on icu4c web site

It is not obvious why the change was made from this message. Let's add
'because of flaky connection failures' or kinda (I don't know exact
problem).
Ok, I've completely rewrote the comment.


> - added patch tool to Docker image bootstrap to be able to patch the
> lzma sources

Should gone with removing lzma hard dependency (see (*)).
Right, it is not needed now - removed.

> - added 'build' directory removement to avoid of old configuration
> at build/curl which is used for curl building
> - removed LD_LIBRARY_PATH environment from curl build, due to the
> path is empty in real and is not needed
> - added '-j' option to make tool calls for all builds to speed it up
> in 4 times
> - set Dockerfile WORKDIR from the very start of Tarantool sources
> builds to make the Dockerfile code more readable and removed all
> duplicating calls to Tarantool sources directory changes.

Let's move all cleanup changes into its own commit. Let's move speed up
to its own commit too.
Created standalone patch set with 5 commits in it for each of the change.


>
> Close #4551
> ---
>
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4551-static-build-full-ci
> Issue: https://github.com/tarantool/tarantool/issues/4551
>
> .gitlab-ci.yml | 12 ++++++++++-
> .gitlab.mk | 7 +++++-
> Dockerfile.staticbuild | 48 ++++++++++++++++++++++++------------------
> cmake/BuildMisc.cmake | 2 +-
> cmake/FindICU.cmake | 2 +-
> cmake/compiler.cmake | 9 ++++++++
> 6 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index cf13c382e..3bdcfa745 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -230,9 +230,19 @@ debian_10:
> OS: 'debian'
> DIST: 'buster'
>
> +# Static builds
> +
> static_build:
> + <<: *release_only_definition
> + <<: *docker_test_definition
> + variables:
> + CMAKE_EXTRA_PARAMS: '-DBUILD_STATIC=ON'
> + script:
> + - ${GITLAB_MAKE} static_build
> +
> +static_docker_build:
> <<: *deploy_test_definition
> variables:
> RUN_TESTS: 'ON'
> script:
> - - ${GITLAB_MAKE} static_build
> + - ${GITLAB_MAKE} static_docker_build
> diff --git a/.gitlab.mk b/.gitlab.mk
> index 48a92e518..a5d4ec26e 100644
> --- a/.gitlab.mk
> +++ b/.gitlab.mk
> @@ -110,5 +110,10 @@ package: git_submodule_update
> # Static build
> # ############
>
> -static_build:
> +deps_debian_static:
> + apt install -y -f liblzma-dev

We use apt-get everywhere in .travis.mk, let's use it here too for
consistency.
Ok, also tried to avoid of it, and found that on Debian Stretch OS the
libunwind static library has undefined use of liblzma functions, because
there is no liblzma static library. Found that libunwind dynamic uses
existing liblzma dynamic library while, and that is why we need to install
liblzma-dev package to additionally provide liblzma static library.
 


> +
> +static_build: deps_debian_static test_debian_no_deps

'test_debian_no_deps' depends on 'build_debian', which doing cmake with
${CMAKE_EXTRA_PARAMS}. Nothing about static build here. Why this make
goal is named static_build?

Yes, I know, CMAKE_EXTRA_PARAMS is set in a gitlab job and there are no
questions about gitlab job name.

Let's remove 'static_build' and 'deps_debian_static' make goals and use
just 'test_debian_no_deps' for 'static_build' gitlab job (with custom
CMAKE_EXTRA_PARAMS)?
Ok, as was discussed by voice decided to move static build make targets from
.gitlab.mk to .travis.mk to store it in common place with the other test/build make
targets. Moved environment from .gitlab-ci.yml file into make targets to

make this targets true building in static w/o additional setup.


> +
> +static_docker_build:
> docker build --network=host --build-arg RUN_TESTS="${RUN_TESTS}" -f Dockerfile.staticbuild .
> diff --git a/Dockerfile.staticbuild b/Dockerfile.staticbuild
> index d63c18bd1..8ca003262 100644
> --- a/Dockerfile.staticbuild
> +++ b/Dockerfile.staticbuild
> @@ -20,6 +20,7 @@ RUN set -x \
> unzip \
> libunwind \
> zlib \
> + patch \
> && yum -y install \
> perl \
> gcc-c++ \
> @@ -44,50 +45,57 @@ RUN set -x && \
> tar -xvf openssl-1.1.0h.tar.gz && \
> cd openssl-1.1.0h && \
> ./config --libdir=lib && \
> - make && make install
> + make -j && make install
>
> RUN set -x && \
> cd / && \
> - wget http://download.icu-project.org/files/icu4c/62.1/icu4c-62_1-src.tgz && \
> + curl -O -L https://github.com/unicode-org/icu/releases/download/release-62-1/icu4c-62_1-src.tgz && \
> tar -xvf icu4c-62_1-src.tgz && \
> cd icu/source && \
> ./configure --with-data-packaging=static --enable-static --enable-shared && \
> - make && make install
> + make -j && make install
>
> RUN set -x && \
> cd / && \
> - LD_LIBRARY_PATH=/usr/local/lib64 curl -O -L http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz && \
> + curl -O -L http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.3-rc1.tar.gz && \
> tar -xvf libunwind-1.3-rc1.tar.gz && \
> cd libunwind-1.3-rc1 && \
> ./configure --enable-static --enable-shared && \
> - make && make install
> + make -j && make install
> +
> +RUN set -x && \
> + mkdir /liblzma && cd /liblzma && \
> + yumdownloader --source xz-devel && \
> + rpm2cpio xz-*.src.rpm | cpio -idv && \
> + tar xf xz-*.tar.gz && \
> + for dir in * ; do cd ${dir} 2>/dev/null && break ; done && \
> + for patch in ../*.patch ; do patch -Np1 < ${patch} ; done && \
> + ./configure --enable-static && \
> + make -j && make install
>
> COPY . /tarantool
>
> +WORKDIR /tarantool
> +
> RUN set -x && \
> - cd tarantool && \
> git submodule init && \
> git submodule update
>
> -WORKDIR /tarantool
> -
> RUN set -x && \
> find . -name 'CMakeFiles' -type d -exec rm -rf {} + && \
> - find . -name 'CMakeCache.txt' -type f -delete
> + find . -name 'CMakeCache.txt' -type f -delete && \
> + rm -rf build
>
> RUN pip install -r /tarantool/test-run/requirements.txt
>
> -RUN set -x \
> - && (cd /tarantool; \
> - cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> - -DENABLE_DIST:BOOL=ON \
> - -DBUILD_STATIC=ON \
> - -DOPENSSL_USE_STATIC_LIBS=ON \
> - -DOPENSSL_ROOT_DIR=/usr/local \
> - .) \
> - && make -C /tarantool -j
> -
> -RUN cd /tarantool && make install
> +RUN set -x && \
> + cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> + -DENABLE_DIST:BOOL=ON \
> + -DBUILD_STATIC=ON \
> + -DOPENSSL_USE_STATIC_LIBS=ON \
> + -DOPENSSL_ROOT_DIR=/usr/local \
> + . && \
> + make -j && make install
>
> ARG RUN_TESTS
> RUN if [ -n "${RUN_TESTS}" ]; then \
> diff --git a/cmake/BuildMisc.cmake b/cmake/BuildMisc.cmake
> index b4a3ca1fc..0c571f326 100644
> --- a/cmake/BuildMisc.cmake
> +++ b/cmake/BuildMisc.cmake
> @@ -39,7 +39,7 @@ macro(libmisc_build)
> else()
> set(GOMP_LIBRARY gomp)
> endif()
> - target_link_libraries(misc ${GOMP_LIBRARY} pthread)
> + target_link_libraries(misc ${GOMP_LIBRARY} pthread dl)
> endif()
>
> unset(misc_src)
> diff --git a/cmake/FindICU.cmake b/cmake/FindICU.cmake
> index 26f0683f3..2bb9a5d59 100644
> --- a/cmake/FindICU.cmake
> +++ b/cmake/FindICU.cmake
> @@ -50,7 +50,7 @@ include(FindPackageHandleStandardArgs)
> find_package_handle_standard_args(ICU
> REQUIRED_VARS ICU_INCLUDE_DIR ICU_LIBRARY_I18N ICU_LIBRARY_UC)
> set(ICU_INCLUDE_DIRS ${ICU_INCLUDE_DIR})
> -set(ICU_LIBRARIES ${ICU_LIBRARY_I18N} ${ICU_LIBRARY_UC} ${ICU_LIBRARY_DATA})
> +set(ICU_LIBRARIES ${ICU_LIBRARY_I18N} ${ICU_LIBRARY_UC} ${ICU_LIBRARY_DATA} dl)
> mark_as_advanced(ICU_INCLUDE_DIR ICU_INCLUDE_DIRS
> ICU_LIBRARY_I18N ICU_LIBRARY_UC ICU_LIBRARIES)
>
> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
> index c9ad2b092..00fac1c66 100644
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -171,6 +171,15 @@ if (ENABLE_BACKTRACE)
> NAMES ${UNWIND_PLATFORM_LIB_NAME})
> set(UNWIND_LIBRARIES ${UNWIND_PLATFORM_LIBRARY} ${UNWIND_LIBRARY})
> endif()
> + if(BUILD_STATIC)
> + set(LZMA_LIB_NAME liblzma.a)
> + find_library(LZMA_LIBRARY PATH_SUFFIXES system NAMES ${LZMA_LIB_NAME})
> + if (NOT LZMA_LIBRARY)
> + message (FATAL_ERROR "ENABLE_BACKTRACE option is set but lzma "
> + "library is not found")

(*)

ENABLE_BACKTRACE does not directly requires liblzma, but only when
libunwind requires it.

Moreover, we don't require lzma anywhere else and, say, does not depend
on it in packages.

What behaviour is desired here: if libunwind requires liblzma (it is
build-time option), then we should always add -llzma after -lunwind (I
assume both after -static), since the latter requires symbols from the
former.

It is nor clear (at least for me) how to determine whether one static
library depends on another in a general case. The approach we use over
cmake files is a kind of workaround: if a library is installed on
default paths, then we add it, because it is harmless even if, say,
libunwind does not depends on liblzma.

Let's adopt this way here and don't give an error if liblzma is not
found.

If you have a vision how to handle transitive dependencies for static
libraries in a better way, let's share it (say, file an issue).
Ok, I've made changes based on your suggestions.

> + endif()
> + set(UNWIND_LIBRARIES ${UNWIND_LIBRARIES} ${LZMA_LIBRARY})
> + endif()
> find_package_message(UNWIND_LIBRARIES "Found unwind" "${UNWIND_LIBRARIES}")
> endif()
>
> --
> 2.17.1
>


--
Alexander Tikhonov