From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9D0E743040D for ; Fri, 4 Sep 2020 20:52:12 +0300 (MSK) Date: Fri, 4 Sep 2020 20:41:43 +0300 From: Igor Munkin Message-ID: <20200904174143.GF18920@tarantool.org> References: <20200825140108.52090-1-huston.mavr@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200825140108.52090-1-huston.mavr@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v2] build: refactor static build process List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: HustonMmmavr Cc: alexander.turenko@tarantool.org, tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@gmail.com Hello, Thanks for the patch! It's strange to me why this patch is not reviewed by Sasha Ti., though there are many changes in the build and testing infrastructure. Nevertheless, please consider my comments below. On 25.08.20, HustonMmmavr wrote: > From: Yaroslav Dynnikov > > Refactored static build process to use static-build/CMakeLists.txt > instead of Dockerfile.staticbuild (this allows to support static > build on macOS). Following third-party dependencies for static build > are installed via cmake `ExternalProject_Add`: > - OpenSSL > - Zlib > - Ncurses > - Readline > - Unwind > - ICU > > * Added support static build for macOS > * Prevented linking tarantool binary with system libcurses (which is > entire copy of libncurses) by setting flag `CURSES_NEED_NCURSES` > to TRUE at file cmake/FindReadline.cmake. (This flag defined at > FindCurses.cmake module and it's a workaround for linking with > correct library) > * Fixed `CONFIGURE_COMMAND` while building bundled libcurl for staic Typo: s/staic/static/. > build at file cmake/BuildLibCURL.cmake: > - disable building shared libcurl libraries (by setting > `--disable-shared` option) > - disable hiding libcurl symbols (by setting > `--disable-symbol-hiding` option) > - prevent linking libcurl with system libz by settign Typo: s/settign/setting/. > `--with-zlib=${FOUND_ZLIB_ROOT_DIR}` option) > * Removed Dockerfile.staticbuild > * Added new gitlab.ci jobs to test new style static build: > - static_build_cmake_linux > - static_build_cmake_osx_15 > * Removed static_docker_build gitlab.ci job > > Closes #5095 > --- > .gitlab-ci.yml | 11 +- > .travis.mk | 52 +++- > Dockerfile.staticbuild | 98 ------- > cmake/BuildLibCURL.cmake | 18 +- > cmake/FindReadline.cmake | 10 + > cmake/compiler.cmake | 24 +- > cmake/os.cmake | 5 +- > static-build/.gitignore | 4 + > static-build/CMakeLists.txt | 240 ++++++++++++++++++ > static-build/README.md | 58 +++++ > static-build/test/CheckDependencies.cmake | 43 ++++ > static-build/test/static-build/box.lua | 3 + > .../test/static-build/exports.test.lua | 148 +++++++++++ > static-build/test/static-build/suite.ini | 5 + > .../test/static-build/traceback.test.lua | 15 ++ > static-build/test/test-run.py | 1 + > 16 files changed, 608 insertions(+), 127 deletions(-) > delete mode 100644 Dockerfile.staticbuild > create mode 100644 static-build/.gitignore > create mode 100644 static-build/CMakeLists.txt > create mode 100644 static-build/README.md > create mode 100644 static-build/test/CheckDependencies.cmake > create mode 100755 static-build/test/static-build/box.lua > create mode 100755 static-build/test/static-build/exports.test.lua > create mode 100644 static-build/test/static-build/suite.ini > create mode 100755 static-build/test/static-build/traceback.test.lua > create mode 120000 static-build/test/test-run.py > > diff --git a/.travis.mk b/.travis.mk > index efc05cf05..482672429 100644 > --- a/.travis.mk > +++ b/.travis.mk > @@ -148,8 +148,12 @@ deps_debian_static: > test_static_build: deps_debian_static > CMAKE_EXTRA_PARAMS=-DBUILD_STATIC=ON make -f .travis.mk test_debian_no_deps > > -test_static_docker_build: > - docker build --no-cache --network=host --build-arg RUN_TESTS=ON -f Dockerfile.staticbuild . > +# New static build > +# builddir used in this target - is a default build path from cmake ExternalProject_Add() The comment exceeds 80 symbols. > +test_static_build_cmake_linux: > diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake > index 5f8b15a63..365c14284 100644 > --- a/cmake/BuildLibCURL.cmake > +++ b/cmake/BuildLibCURL.cmake > @@ -4,15 +4,20 @@ macro(curl_build) > if (BUILD_STATIC) > - set(LIBZ_LIB_NAME libz.a) > + find_library(LIBZ_LIBRARY NAMES libz.a) > else() > - set(LIBZ_LIB_NAME z) > + find_library(LIBZ_LIBRARY NAMES z) > endif() > - find_library(LIBZ_LIBRARY NAMES ${LIBZ_LIB_NAME}) Well... What is the difference from the previous approach? > - if ("${LIBZ_LIBRARY}" STREQUAL "LIBZ_LIBRARY-NOTFOUND") > + message(STATUS "Looking for libz - ${LIBZ_LIBRARY}") > + > diff --git a/static-build/.gitignore b/static-build/.gitignore > new file mode 100644 > index 000000000..c8028a870 > --- /dev/null > +++ b/static-build/.gitignore Minor: Why did you introduce a separate .gitignore instead of adjusting the root one? > @@ -0,0 +1,4 @@ > diff --git a/static-build/CMakeLists.txt b/static-build/CMakeLists.txt > new file mode 100644 > index 000000000..d90a642e6 > --- /dev/null > +++ b/static-build/CMakeLists.txt > @@ -0,0 +1,240 @@ > +cmake_minimum_required(VERSION 2.8) > + > +project(tarantool-env NONE) Minor: Why is the name tarantool-env and not e.g. tarantool-static? > + > + > +if (APPLE) > + find_program(C_COMPILER clang) > + find_program(CXX_COMPILER clang++) > +else() > + find_program(C_COMPILER gcc) > + find_program(CXX_COMPILER g++) > +endif() > +set(CMAKE_C_COMPILER ${C_COMPILER}) > +set(CMAKE_CXX_COMPILER ${CXX_COMPILER}) I see you've added a comment for this passage, *but* at least for me Sasha's comment is still left unaddressed. AFAICS Sasha asked whether we can solve the linkage issue by explicitly setting CMAKE_C{,XX}_COMPILER variables. If the problem isn't fixed this way, then I guess your solution is fine. > + > +# Install all libraries required by tarantool at current build dir > + > +# > +# ICU > +# > +ExternalProject_Add(icu > + URL https://github.com/unicode-org/icu/releases/download/release-62-1/icu4c-62_1-src.tgz > + CONFIGURE_COMMAND CC=${CMAKE_C_COMPILER} > + CXX=${CMAKE_CXX_COMPILER} OK, I see, you've already set the variables I mentioned above. Anyway, please mention this fact if it doesn't fix the linkage issue (maybe refer the corresponding place in libicu configure / the issue / etc). > + /source/configure > + --with-data-packaging=static > + --prefix= > + --disable-shared > + --enable-static > +) > + > +# > +# ZLIB > +# > +ExternalProject_Add(zlib > + URL https://zlib.net/zlib-${ZLIB_VERSION}.tar.gz > + CONFIGURE_COMMAND CC=${CMAKE_C_COMPILER} > + /configure > + --prefix= > + --static > + TEST_COMMAND ${CMAKE_MAKE_PROGRAM} check This TEST_COMMAND line looks odd and I guess it need some commenting with the rationale, since other commands don't have it. > +) > + > + > +# > +# ReadLine > +# > +ExternalProject_Add(readline > + URL https://ftp.gnu.org/gnu/readline/readline-${READLINE_VERSION}.tar.gz > + CONFIGURE_COMMAND CC=${CMAKE_C_COMPILER} > + /configure > + --prefix= > + --disable-shared > + # STEP_TARGETS download Looks like this line can be dropped. > +) > + > diff --git a/static-build/README.md b/static-build/README.md > new file mode 100644 > index 000000000..0019e963f > --- /dev/null > +++ b/static-build/README.md > @@ -0,0 +1,58 @@ > +# Tarantool static build tooling > + > +These files help to prepare environment for building Tarantool > +statically. And builds it. > + > +## Prerequisites > + > +CentOS: Minor: Does this mean that other distros are not supported? > + > +```bash > +yum install -y \ > + git perl gcc cmake make gcc-c++ libstdc++-static autoconf automake libtool \ > + python-msgpack python-yaml python-argparse python-six python-gevent > +``` > + > +MacOS: > + > +Before you start please install default Xcode Tools by Apple: > + > +``` Typo: bash specifier is missing (considering verbatim blocks below). > +sudo xcode-select --install > +sudo xcode-select -switch /Applications/Xcode.app/Contents/Developer > +``` > + > +Install brew using command from > +[Homebrew repository instructions](https://github.com/Homebrew/inst) > + > +After that run next script: > + > +```bash > + brew install autoconf automake libtool cmake file://$${PWD}/tools/brew_taps/tntpython2.rbs > + pip install --force-reinstall -r test-run/requirements.txt Typo: Looks like misindentation. > +``` > + > +### Usage Typo: I guess it should be h2, not h3. > + > +```bash > +cmake . > +make -j > +ctest -V > +``` > + > +## Customize your build > + > +If you want to customise build, you need to set `CMAKE_TARANTOOL_ARGS` variable > + > +### Usage > + > +There is three types of `CMAKE_BUILD_TYPE`: Typo: s/is/are/. > +* Debug - default > +* Release > +* RelWithDebInfo > + > +And you want to build tarantool with RelWithDebInfo: > + > +```bash > +cmake -DCMAKE_TARANTOOL_ARGS="-DCMAKE_BUILD_TYPE=RelWithDebInfo" . > +``` > diff --git a/static-build/test/static-build/exports.test.lua b/static-build/test/static-build/exports.test.lua > new file mode 100755 > index 000000000..63dc163a9 > --- /dev/null > +++ b/static-build/test/static-build/exports.test.lua > @@ -0,0 +1,148 @@ > + > +local RTLD_DEFAULT > +-- See `man 3 dlsym`: > +-- RTLD_DEFAULT > +-- Find the first occurrence of the desired symbol using the default > +-- shared object search order. The search will include global symbols Typo: There are several places above with doubled whitespace. > +-- in the executable and its dependencies, as well as symbols in shared > +-- objects that were dynamically loaded with the RTLD_GLOBAL flag. > +if jit.os == "OSX" then > + RTLD_DEFAULT = ffi.cast("void *", -2LL) > +else > + RTLD_DEFAULT = ffi.cast("void *", 0LL) > +end Minor: OK, since the difference is only in the constant value, the following way looks more convenient to me: | local RTLD_DEFAULT = ffi.cast("void *", jit.os == "OSX" and -2LL or 0LL) > + > -- > 2.26.2 > -- Best regards, IM