Re: [Tarantool-patches] [PATCH] build: refactor static build process

[Alexander Turenko <alexander.turenko@tarantool.org>]

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=<INSTALL_DIR>/var

To be honest, I don't understand what will going wrong if we'll not set
the variable.

> +        -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
> +        -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