From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 4B799445320 for ; Tue, 28 Jul 2020 01:42:17 +0300 (MSK) Date: Tue, 28 Jul 2020 01:41:22 +0300 From: Alexander Turenko Message-ID: <20200727223734.cnxrdzik2cyt3ey4@tkn_work_nb> References: <20200622181649.10100-1-huston.mavr@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200622181649.10100-1-huston.mavr@gmail.com> Subject: Re: [Tarantool-patches] [PATCH] build: refactor static build process List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: HustonMmmavr Cc: tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@gmail.com It is not review in fact: the patch is not so small and I need to start several discussions here and there to dive into the changes deeply and finally provide thorough review. So, please consider the comments below not as 'you should do X and Y to pass the review', but as questions from a curious person. WBR, Alexander Turenko. > 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 How about libicu? I guess it is not part of Mac OS system itself. > > * Added support static build for macOS > * Prevented linking tarantool binary with system libcurses.dylib at > macOS by setting flag `CURSES_NEED_NCURSES` to TRUE at file > cmake/FindReadline.cmake First, it is set for both Linux and Max OS without any comment in the code. Second, it is unclear what is bad with linking against libcurses.dylib. The only way to find a reason is to try and got some fail. Please, clarify it. > * Fixed `CONFIGURE_COMMAND` while building bundled libcurl for staic > build at file cmake/BuildLibCURL.cmake: Typo: staic. > - disable building shared libcurl libraries (by setting > `--disable-shared` option) Is it necessary? I had a plan to use .so to grab a symbol list to export. > * Added new gitlab.ci jobs to test new style static build: > - static_build_no_deps_linux > - static_build_no_deps_osx_15 'no_deps' is suffix for make targets, which do not depend on 'deps_debian' or 'deps_buster_clang_8'. A CI job may either use a docker image, where all those dependencies are already installed, or install dependencies each time (which is slower). In any way there is no reason to name a CI job as 'no_deps'. Can we add a Linux/clang job to better understand what is Mac OS specific problem and what is related to clang itself? > +# New static build Nit: I'll become the only static build after your patch, so I don't see a reason to highlight the fact the it is the new one. > +test_static_build_no_deps_linux: > + cd static-build && cmake . && make -j && ctest -V > + cd test && /usr/bin/python test-run.py --force \ > + --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-build $(TEST_RUN_EXTRA_PARAMS) To be honest, I don't find from where tarantool-prefix appears. Some ExternalProject_Add() default? BTW, I personally prefer to have separate workdir and destdir: not workdir inside destdir (example: emerge in Gentoo organizes build files in the similar way). And I also feel the name 'dest' (borrowed from DESTDIR, which is usual GNU Make variable name) more descriptive, then 'prefix'. 'prefix' is how you want to use it ('to prefix other variables'), but 'destination directory' is what the directory is. You can see how everything is organized in cmake/BuildLibCURL.cmake. (It is just my preferences, but maybe it'll look meaningful for you too. I don't insist.) > +test_osx_no_deps: build_osx > + # Init macOS test env > + ${INIT_TEST_ENV_OSX}; \ The comment just repeats the variable name. It is useless. > + # Init macOS test env > + ${INIT_TEST_ENV_OSX}; \ Same as above, the comment just repeats the variable name. > 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}) The code block does exactly same before and after the change: so there is no need to change it. > +if(BUILD_STATIC AND NOT TARGET_OS_DARWIN) > set(UNWIND_LIB_NAME libunwind.a) > else() > set(UNWIND_LIB_NAME unwind) A comment (like in static-build/CMakeLists.txt) or some link to the existing comment should be here, it is not obvious why we should link against libunwind dynamically on Mac OS. > -if(BUILD_STATIC) > +if(BUILD_STATIC AND NOT TARGET_OS_DARWIN) > # Static linking for c++ routines > add_compile_flags("C;CXX" "-static-libstdc++") > endif() There should a clear reason for this change (say, it does not work with symptoms X and Y) and a reason why it is okay on Mac OS (say, this runtime exist on any Mac OS system, even without developer tool, and ABI is stable). Please, comment those moments. Otherwise, if we'll want to change it in a future, we'll not know what may become broken and what need to be verified. > +find_program(C_COMPILER gcc) > +find_program(CXX_COMPILER g++) > +set(CMAKE_C_COMPILER ${C_COMPILER}) > +set(CMAKE_CXX_COMPILER ${CXX_COMPILER}) It is intersting. Do you want to use gcc even on Mac OS? Why don't just use a default system compiler? > +# > +# ICONV > +# > +if (NOT APPLE) Nit: It is easier to read a condition | if (NOT <..FOO..>) | <..AAA..> | else() | <..BBB..> | endif() when it is written as: | if (<..FOO..>) | <..BBB..> | else() | <..AAA..> | endif() > +# > +# Unwind > +# > +if (APPLE) > + # On macOS libunwind is a part of MacOSX.sdk > + # So we need to find library and header and > + # copy it locally Is this SDK always available on a target system? Or it assumes some developments tools to be installed? I would clarify it in the README, if so. > + add_custom_target(unwind DEPENDS ${UNWIND_DEPENDENCIES}) > + set_target_properties(unwind > + PROPERTIES _EP_INSTALL_DIR ${UNWIND_INSTALL_PREFIX} > + ) What is _EP_INSTALL_DIR? I don't see anything about in in the CMake documentation ([1]). If it is uses some undocumented CMake feature, then there should be a comment. [1]: https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html > +ExternalProject_Add(tarantool > + DEPENDS ${TARANTOOL_DEPENDS} > + SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/.. > + LIST_SEPARATOR : > + CMAKE_ARGS > + # Override LOCALSTATEDIR to avoid cmake "special" cases: > + # https://cmake.org/cmake/help/v3.4/module/GNUInstallDirs.html#special-cases > + -DCMAKE_INSTALL_LOCALSTATEDIR=/var To be honest, I don't understand what will going wrong if we'll not set the variable. > + -DCMAKE_INSTALL_PREFIX= > + -DCMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH} > + -DCMAKE_FIND_USE_CMAKE_SYSTEM_PATH=FALSE So find_*() will search only in ${CMAKE_PREFIX_PATH} directories? Now I got why you need to copy system's libunwind.dylib into a local directory. It gives you more control what parts of a system may be linked dynamically into the executable, so I think it is good approach. > + -DOPENSSL_USE_STATIC_LIBS=TRUE > + -DCMAKE_BUILD_TYPE=Debug Maybe allow to redefine those options? However it seems it is possible using -DCMAKE_TARANTOOL_ARGS=<...>. I think it would be more convenient to accept just, say, -DCMAKE_BUILD_TYPE=RelWithDebInfo. Either way you'll choose, please, document it is static-build/README.md. > + -DBUILD_STATIC=TRUE > + -DENABLE_DIST=TRUE > + -DENABLE_BACKTRACE=TRUE > + -DPACKAGE:STRING=${PACKAGE_NAME} Nit: Other variables are set without a type. > + -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} > + -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} > + ${CMAKE_TARANTOOL_ARGS} > + BUILD_COMMAND ${CMAKE_MAKE_PROGRAM} -j AFAIR, ${MAKE} should keep -j value (it is used in cmake/BuildLibCURL.cmake). Does not ${CMAKE_MAKE_PROGRAM} do? > +## Prerequisites > + > +CentOS: > + > +```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 > +``` > + I would mention Mac OS requirements. Even if it is 'any Mac OS system', it is great thing to highlight. > +local check_symbols = { > + -- FFI > + > + 'guava', > + 'base64_decode', > + 'base64_encode', > + 'SHA1internal', > + 'random_bytes', > + 'fiber_time', > + 'ibuf_create', > + 'ibuf_destroy', > + 'port_destroy', > + 'csv_create', > + 'csv_destroy', > + 'title_get', > + 'title_update', > + 'tnt_iconv', > + 'tnt_iconv_open', > + 'tnt_iconv_close', > + 'exception_get_int', > + 'exception_get_string', > + > + 'tarantool_lua_ibuf', > + 'uuid_nil', > + 'tt_uuid_create', > + 'tt_uuid_str', > + 'tt_uuid_is_equal', > + 'tt_uuid_is_nil', > + 'tt_uuid_bswap', > + 'tt_uuid_from_string', > + 'log_level', > + 'log_format', > + 'uri_parse', > + 'uri_format', > + 'PMurHash32', > + 'PMurHash32_Process', > + 'PMurHash32_Result', > + 'crc32_calc', > + 'mp_encode_double', > + 'mp_encode_float', > + 'mp_encode_decimal', > + 'mp_decode_double', > + 'mp_decode_float', > + 'mp_decode_extl', > + 'mp_sizeof_decimal', > + 'decimal_unpack', > + > + 'log_type', > + 'say_set_log_level', > + 'say_logrotate', > + 'say_set_log_format', > + 'tarantool_uptime', > + 'tarantool_exit', > + 'log_pid', > + 'space_by_id', > + 'space_run_triggers', > + 'space_bsize', > + 'box_schema_version', > + > + 'crypto_EVP_MD_CTX_new', > + 'crypto_EVP_MD_CTX_free', > + 'crypto_HMAC_CTX_new', > + 'crypto_HMAC_CTX_free', > + 'crypto_stream_new', > + 'crypto_stream_begin', > + 'crypto_stream_append', > + 'crypto_stream_commit', > + 'crypto_stream_delete', > + > + -- Module API > + > + '_say', > + 'swim_cfg', > + 'swim_quit', > + 'fiber_new', > + 'fiber_cancel', > + 'coio_wait', > + 'coio_close', > + 'coio_call', > + 'coio_getaddrinfo', > + 'luaT_call', > + 'box_txn', > + 'box_select', > + 'clock_realtime', > + 'string_strip_helper', > + > + -- Lua / LuaJIT > + > + 'lua_newstate', > + 'lua_close', > + 'luaL_loadstring', > + 'luaJIT_profile_start', > + 'luaJIT_profile_stop', > + 'luaJIT_profile_dumpstack', > + > + 'ERR_error_string', > + 'ERR_get_error', > + > + 'EVP_get_digestbyname', > + 'EVP_get_cipherbyname', > + 'EVP_CIPHER_CTX_new', > + 'EVP_CIPHER_CTX_free', > + 'EVP_CIPHER_block_size', > + 'HMAC_Init_ex', > + 'HMAC_Update', > + 'HMAC_Final', > + > + 'ZSTD_compress', > + 'ZSTD_decompress', > + 'ZSTD_free', > + 'ZSTD_malloc', > + 'ZSTD_versionString', > +} The list is based on src/exports.h (and old extra/exports)? Or it is requirements of some set of external modules? Please, clarify. > diff --git a/static-build/test/static-build/suite.ini b/static-build/test/static-build/suite.ini > new file mode 100644 > index 000000000..4da3d5d2f > --- /dev/null > +++ b/static-build/test/static-build/suite.ini > @@ -0,0 +1,6 @@ > +[default] > +core = app > +description = Static build tests > +script = box.lua > +is_parallel = True > +use_unix_sockets_iproto = True 'use_unix_sockets_iproto' will not affect anything, because of two reasons: - It does not work for 'core = app' test (see [1]). Just was not implemented for them. I don't recommend to enable it for 'core = app' test suites prematurely, because a test-run update may break your tests in the case. - You don't use 'LISTEN' environment variable. But you may be interested in use_unix_sockets option (but take care to max usix socket path limit). [1]: https://github.com/tarantool/test-run/issues/141#issuecomment-618401440