[Tarantool-patches] [PATCH v1] build: fix static build

[Alexander Turenko]

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.

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.

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

Commented below (see (*)).

>  - 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?

> 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.

> 
> 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.

>  - 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).

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

Should gone with removing lzma hard dependency (see (*)).

>  - 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.

> 
> 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.

> +
> +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)?

> +
> +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).

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