[tarantool-patches] Re: [PATCH] Tarantool static build
Alexander Turenko
alexander.turenko at tarantool.org
Tue Aug 14 17:58:52 MSK 2018
Hi!
Common note:
Can we wrap find_package to some macro that will pay attention to
BUILD_STATIC value. Even if not all cases will be covered with this
approach, it seems that most of them will do.
Inline comments are below. Mostly they are requests to describe used
approach. Some are questions or proposals I don't sure about. Please,
don't consider this as nitpicking.
WBR, Alexander Turenko.
On Thu, Aug 02, 2018 at 11:13:34AM +0300, Georgy Kirichenko wrote:
> A possibility to build tarantool with included library dependency.
> Use the flag -DBUILD_STATIC=NULL to build statically against curl, readline,
> ncurses, icu and z.
Why NULL? As I know such options usually are ON/OFF.
> Use the flag -DOPENSSL_USE_TATIC_LIBS=ON to build with static
> openssl
TATIC -> STATIC
>
> Note: Bundled libyaml is not properly exported, use the system one.
>
> Fixes #3445
> ---
> Branch: https://github.com/tarantool/tarantool/tree/g.kirichenko/static-build
> Issue: https://github.com/tarantool/tarantool/issues/3445
>
> CMakeLists.txt | 51 +++--
> cmake/BuildMisc.cmake | 11 +
> cmake/FindCURL.cmake | 13 +-
> cmake/FindICU.cmake | 18 +-
> cmake/FindLibYAML.cmake | 6 +-
> cmake/FindOpenSSL.cmake | 465 +++++++++++++++++++++++++++++++++++++++
> cmake/FindReadline.cmake | 19 +-
> cmake/FindTermcap.cmake | 6 +-
> cmake/FindZSTD.cmake | 5 +-
> cmake/compiler.cmake | 31 ++-
> extra/exports | 17 ++
> extra/mkexports | 4 +-
> src/CMakeLists.txt | 50 ++++-
> test/app-tap/suite.ini | 1 +
> third_party/crc32.c | 6 +-
> third_party/crc32.h | 6 +-
> 16 files changed, 649 insertions(+), 60 deletions(-)
> create mode 100644 cmake/FindOpenSSL.cmake
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index d0cae0f01..a1dfb10b0 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -286,6 +286,31 @@ find_package_message(MODULE_LIBPATH "Lua package.cpath: ${MODULE_LIBPATH}"
>
> add_custom_target(build_bundled_libs)
>
> +# Debian: missing zstd_static.h in libzstd-dev
> +# Fedora: not found
> +# => use bundled version by default
> +
> +option(ENABLE_BUNDLED_ZSTD "Enable building of the bundled zstd" ON)
> +if (ENABLE_BUNDLED_ZSTD)
> + include(BuildZSTD)
> + zstd_build()
> + add_dependencies(build_bundled_libs zstd)
> +else()
> + set(ZSTD_FIND_REQUIRED ON)
> + find_package(ZSTD)
> +endif()
> +
> +#
> +# OpenSSL
> +#
> +find_package(OpenSSL)
> +if (OPENSSL_FOUND)
> + message(STATUS "OpenSSL ${OPENSSL_VERSION} found")
> + include_directories(${OPENSSL_INCLUDE_DIR})
> +else()
> + message(FATAL_ERROR "Could NOT find OpenSSL development files (libssl-dev/openssl-devel package)")
> +endif()
> +
> #
> # Curl
> #
> @@ -398,32 +423,6 @@ endif()
> # zstd
> #
>
> -# Debian: missing zstd_static.h in libzstd-dev
> -# Fedora: not found
> -# => use bundled version by default
> -
> -option(ENABLE_BUNDLED_ZSTD "Enable building of the bundled zstd" ON)
> -if (ENABLE_BUNDLED_ZSTD)
> - include(BuildZSTD)
> - zstd_build()
> - add_dependencies(build_bundled_libs zstd)
> -else()
> - set(ZSTD_FIND_REQUIRED ON)
> - find_package(ZSTD)
> -endif()
> -
> -#
> -# OpenSSL
> -#
> -
> -find_package(OpenSSL)
> -if (OPENSSL_FOUND)
> - message(STATUS "OpenSSL ${OPENSSL_VERSION} found")
> - include_directories(${OPENSSL_INCLUDE_DIR})
> -else()
> - message(FATAL_ERROR "Could NOT find OpenSSL development files (libssl-dev/openssl-devel package)")
> -endif()
> -
Why do you need to change order of find_package? If there is some
specific order we should follow (say, because of curl depends on
openssl) it would be good to mention that in the commit message. And I
hope there is more proper way to handle dependencies in cmake (instead
of ordering).
> #
> # Third-Party misc
> #
> diff --git a/cmake/BuildMisc.cmake b/cmake/BuildMisc.cmake
> index 20ecb4f63..22b758c42 100644
> --- a/cmake/BuildMisc.cmake
> +++ b/cmake/BuildMisc.cmake
> @@ -33,5 +33,16 @@ macro(libmisc_build)
>
> add_library(misc STATIC ${misc_src})
>
> + if (HAVE_OPENMP)
> + target_compile_options(misc PRIVATE "-fopenmp")
> + if(BUILD_STATIC)
Tab symbol here instead of whitespaces.
> + set(GOMP_LIBRARY libgomp.a)
> + else()
> + set(GOMP_LIBRARY gomp)
> + endif()
> + target_link_libraries(misc pthread ${GOMP_LIBRARY})
> + endif()
> +
> +
> unset(misc_src)
> endmacro(libmisc_build)
> <...>
> diff --git a/cmake/FindLibYAML.cmake b/cmake/FindLibYAML.cmake
> index 8abe5f9f0..11de19165 100644
> --- a/cmake/FindLibYAML.cmake
> +++ b/cmake/FindLibYAML.cmake
> @@ -2,8 +2,12 @@ find_path(LIBYAML_INCLUDE_DIR
> NAMES yaml.h
> )
>
> +if(BUILD_STATIC)
> + set(YAML_STATIC libyaml.a)
> +endif()
> +
> find_library(LIBYAML_LIBRARY
> - NAMES yaml
> + NAMES ${YAML_STATIC} yaml
Tab symbol instead of whitespaces.
> )
>
> set(LIBYAML_INCLUDE_DIRS "${LIBYAML_INCLUDE_DIR}")
> diff --git a/cmake/FindOpenSSL.cmake b/cmake/FindOpenSSL.cmake
> new file mode 100644
> index 000000000..f4579b465
> --- /dev/null
> +++ b/cmake/FindOpenSSL.cmake
As I see it is copy of [1] with the following patch applied:
--- FindOpenSSL.cmake.prev 2018-08-02 20:45:11.023410550 +0300
+++ cmake/FindOpenSSL.cmake 2018-08-02 19:43:03.529563407 +0300
@@ -371,7 +371,7 @@
endif ()
endif ()
-include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake)
+include(FindPackageHandleStandardArgs)
set(OPENSSL_LIBRARIES ${OPENSSL_SSL_LIBRARY} ${OPENSSL_CRYPTO_LIBRARY} )
Note: it is not the last version of the file. The commit [2] was made
since that one.
I don't sure that copying files from cmake distribution is right way to
handle dependencies (I have the file on my system in the path [3]). But
it seems to be approach we already use for other packages and maybe is
worth to hold certain version of the cmake file.
[1]: https://gitlab.kitware.com/cmake/cmake/blob/c468b9a2314a67f9d697d27c12f9c39e1301f1aa/Modules/FindOpenSSL.cmake
[2]: https://gitlab.kitware.com/cmake/cmake/commit/912a6c1cb5949fafe8b17216f162a16261d59d0c
[3]: /usr/share/cmake/Modules/FindOpenSSL.cmake
> @@ -0,0 +1,465 @@
> +# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
> +# file Copyright.txt or https://cmake.org/licensing for details.
> +
> <...>
> diff --git a/cmake/FindReadline.cmake b/cmake/FindReadline.cmake
> index 681a6f5de..e347c4cd3 100644
> --- a/cmake/FindReadline.cmake
> +++ b/cmake/FindReadline.cmake
> @@ -6,22 +6,33 @@
> # READLINE_LIBRARIES
> #
>
> +if(BUILD_STATIC)
> + find_library(CURSES_CURSES_LIBRARY NAMES libcurses.a)
> + find_library(CURSES_NCURSES_LIBRARY NAMES libncurses.a)
> + find_library(CURSES_FORM_LIBRARY NAMES libform.a)
> + find_library(CURSES_INFO_LIBRARY NAMES libtinfo.a)
> + if (NOT CURSES_INFO_LIBRARY)
> + set(CURSES_INFO_LIBRARY "")
Tab symbol instead of whitespaces.
> + endif()
> +endif()
> find_package(Curses)
> if(NOT CURSES_FOUND)
> find_package(Termcap)
> endif()
>
> +set(READLINE_STATIC libreadline.a)
In other files you are set *_STATIC variables under if(BUILD_STATIC),
but here you don't. Is it intentional?
> +
> if (DEFINED READLINE_ROOT)
> set(_FIND_OPTS NO_CMAKE NO_CMAKE_SYSTEM_PATH)
> find_library(READLINE_LIBRARY
> - NAMES readline
> + NAMES ${READLINE_STATIC} readline
> HINTS ${READLINE_ROOT}/lib
> ${_FIND_OPTS})
> find_path(READLINE_INCLUDE_DIR
> NAMES readline/readline.h
> HINTS ${READLINE_ROOT}/include ${_FIND_OPTS})
> else()
> - find_library(READLINE_LIBRARY NAMES readline)
> + find_library(READLINE_LIBRARY NAMES ${READLINE_STATIC} readline)
> find_path(READLINE_INCLUDE_DIR readline/readline.h)
> endif()
>
> @@ -41,10 +52,10 @@ if(READLINE_FOUND)
> endif()
> endif()
> if(CURSES_FOUND)
> - set(READLINE_LIBRARIES ${READLINE_LIBRARIES} ${CURSES_LIBRARIES})
> + set(READLINE_LIBRARIES ${READLINE_LIBRARIES} ${CURSES_LIBRARIES} ${CURSES_INFO_LIBRARY})
> set(READLINE_INCLUDE_DIRS ${READLINE_INCLUDE_DIRS} ${CURSES_INCLUDE_DIRS})
> elseif(TERMCAP_FOUND)
> - set(READLINE_LIBRARIES ${READLINE_LIBRARIES} ${TERMCAP_LIBRARIES})
> + set(READLINE_LIBRARIES ${READLINE_LIBRARIES} ${TERMCAP_LIBRARIES} ${CURSES_INFO_LIBRARY})
> set(READLINE_INCLUDE_DIRS ${READLINE_INCLUDE_DIRS} ${TERMCAP_INCLUDE_DIRS})
> endif()
> endif(READLINE_FOUND)
> <...>
> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
> index 05d33ab48..4f9c2c049 100644
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -116,7 +116,10 @@ set (CMAKE_CXX_FLAGS_RELWITHDEBINFO
> unset(CC_DEBUG_OPT)
>
> check_include_file(libunwind.h HAVE_LIBUNWIND_H)
> -find_library(UNWIND_LIBRARY PATH_SUFFIXES system NAMES unwind)
> +if(BUILD_STATIC)
> + set(UNWIND_STATIC libunwind.a)
> +endif()
> +find_library(UNWIND_LIBRARY PATH_SUFFIXES system NAMES ${UNWIND_STATIC} unwind)
>
> set(ENABLE_BACKTRACE_DEFAULT OFF)
> if (UNWIND_LIBRARY AND HAVE_LIBUNWIND_H)
> @@ -137,20 +140,33 @@ if (ENABLE_BACKTRACE)
> else()
> if (CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64" OR
> CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> + if(BUILD_STATIC)
> + set(UNWIND_PLATFORM_STATIC "libunwind-${CMAKE_SYSTEM_PROCESSOR}.a")
> + endif()
> find_library(UNWIND_PLATFORM_LIBRARY PATH_SUFFIXES system
> - NAMES "unwind-${CMAKE_SYSTEM_PROCESSOR}")
> + NAMES ${UNWIND_PLATFORM_STATIC}
> + "unwind-${CMAKE_SYSTEM_PROCESSOR}")
Tabs instead of whitespaces.
> elseif (CMAKE_SYSTEM_PROCESSOR STREQUAL "i686")
> + if(BUILD_STATIC)
> + set(UNWIND_PLATFORM_STATIC "libunwind-x86.a")
> + endif()
> find_library(UNWIND_PLATFORM_LIBRARY PATH_SUFFIXES system
> - NAMES "unwind-x86")
> + NAMES ${UNWIND_PLATFORM_STATIC} "unwind-x86")
> elseif (CMAKE_SYSTEM_PROCESSOR MATCHES "arm*")
> + if(BUILD_STATIC)
> + set(UNWIND_PLATFORM_STATIC "libunwind-arm.a")
> + endif()
> find_library(UNWIND_PLATFORM_LIBRARY PATH_SUFFIXES system
> - NAMES "unwind-arm")
> + NAMES ${UNWIND_PLATFORM_STATIC} "unwind-arm")
> endif()
> - set (UNWIND_LIBRARIES ${UNWIND_LIBRARY} ${UNWIND_PLATFORM_LIBRARY})
> + set (UNWIND_LIBRARIES ${UNWIND_PLATFORM_LIBRARY} ${UNWIND_LIBRARY})
> endif()
> find_package_message(UNWIND_LIBRARIES "Found unwind" "${UNWIND_LIBRARIES}")
> endif()
>
> +# Static linking for c++ routines
> +add_compile_flags("C;CXX" "-static-libgcc" "-static-libstdc++")
> +
> #
> # Set flags for all include files: those maintained by us and
> # coming from third parties.
> @@ -259,11 +275,6 @@ macro(enable_tnt_compile_flags)
> endif()
> endmacro(enable_tnt_compile_flags)
>
> -if (HAVE_OPENMP)
> - add_compile_flags("C;CXX" "-fopenmp")
> -endif()
> -
> -
> if (CMAKE_COMPILER_IS_CLANG OR CMAKE_COMPILER_IS_GNUCC)
> set(HAVE_BUILTIN_CTZ 1)
> set(HAVE_BUILTIN_CTZLL 1)
> diff --git a/extra/exports b/extra/exports
> index 61a2e0a54..a93197e2c 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -79,6 +79,23 @@ tnt_EVP_MD_CTX_new
> tnt_EVP_MD_CTX_free
> tnt_HMAC_CTX_new
> tnt_HMAC_CTX_free
> +EVP_DigestInit_ex
> +EVP_DigestUpdate
> +EVP_DigestFinal_ex
> +EVP_get_digestbyname
> +HMAC_Init_ex
> +HMAC_Update
> +HMAC_Final
> +EVP_CIPHER_CTX_new
> +EVP_CIPHER_CTX_free
> +EVP_CipherInit_ex
> +EVP_CipherUpdate
> +EVP_CipherFinal_ex
> +EVP_CIPHER_CTX_cleanup
> +EVP_CIPHER_block_size
> +EVP_get_cipherbyname
> +ERR_get_string
> +ERR_error_string
>
> # Module API
>
> diff --git a/extra/mkexports b/extra/mkexports
> index 20b3454d4..88e2e189f 100755
> --- a/extra/mkexports
> +++ b/extra/mkexports
> @@ -2,6 +2,7 @@
> # $1 - in file
> # $2 - out file
> # $3 - os
> +# $4 - export templates
> if [ "x$3x" = xDarwinx ]; then
> # _func1
> # _func2
> @@ -11,5 +12,6 @@ else
> # func1;
> # func2;
> # };
> - ( echo "{" && sed -e '/^\s*$/d;s/$/;/;' $1 && echo "};" ) > $2
> + echo "$4"
> + ( echo "{" && { cat $1 ; for so in $4 ; do { nm -D $so || nm -D `cat $so | grep GROUP | awk '{print $3}'` ; } | awk '{print $3}' ; done ; } | sed '/^\s*$/d;s/$/;/;' && echo "};" ) > $2
What is the root of differences btw these variants?
1. nm -D $so
2. nm -D `cat $so | grep GROUP | awk '{print $3}'`
> fi
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 2a952923e..82f173605 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -107,18 +107,19 @@ endif ()
>
> add_library(core STATIC ${core_sources})
> target_link_libraries(core
> - salad small pthread
> + salad small
> ${LIBEV_LIBRARIES}
> ${LIBEIO_LIBRARIES}
> ${LIBCORO_LIBRARIES}
> ${MSGPUCK_LIBRARIES}
> + dl pthread
> )
>
> add_library(stat STATIC rmean.c latency.c histogram.c)
> target_link_libraries(stat core)
>
> if (ENABLE_BACKTRACE AND NOT TARGET_OS_DARWIN)
> - target_link_libraries(core gcc_s ${UNWIND_LIBRARIES})
> + target_link_libraries(core gcc_s ${UNWIND_LIBRARIES})
> endif()
>
> if (CC_HAS_WNO_IMPLICIT_FALLTHROUGH)
> @@ -218,12 +219,18 @@ target_link_libraries(server core bit uri uuid ${ICU_LIBRARIES})
> set (reexport_libraries server core misc bitset csv
> ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES})
>
> +if(BUILD_STATIC)
> + set(Z_STATIC libz.a)
> +endif()
> +find_library(Z_LIBRARY NAMES ${Z_STATIC} z)
> +
> set (common_libraries
> ${reexport_libraries}
> ${LIBYAML_LIBRARIES}
> ${READLINE_LIBRARIES}
> - ${OPENSSL_LIBRARIES}
> ${CURL_LIBRARIES}
> + ${OPENSSL_LIBRARIES}
> + ${Z_STATIC}
> )
>
> if (TARGET_OS_LINUX OR TARGET_OS_DEBIAN_FREEBSD)
> @@ -248,6 +255,8 @@ endif()
> set (common_libraries ${common_libraries} ${LIBUUID_LIBRARIES})
> set (common_libraries ${common_libraries} PARENT_SCOPE)
>
> +set (generic_libraries pthread dl)
> +
> add_subdirectory(lib)
> add_subdirectory(box)
>
> @@ -255,6 +264,29 @@ add_subdirectory(box)
> set(TARANTOOL_C_FLAGS ${CMAKE_C_FLAGS} PARENT_SCOPE)
> set(TARANTOOL_CXX_FLAGS ${CMAKE_CXX_FLAGS} PARENT_SCOPE)
>
> +set(EXPORT_LIST)
> +if(BUILD_STATIC)
> + foreach(libstatic
> + ${READLINE_LIBRARIES}
> + ${CURL_LIBRARIES}
> + ${OPENSSL_LIBRARIES}
> + ${ICU_LIBRARIES}
> + ${Z_STATIC}
> + ${LIBYAML_LIBRARIES})
> + if (${libstatic} MATCHES "lib[^/]+.a")
> + string(REGEX MATCH "lib[^/]+.a" libname ${libstatic})
> + string(REGEX REPLACE "\\.a$" "" libname ${libname})
> + string(REGEX REPLACE "^lib" "" libname ${libname})
> + find_library(SYMBOLS_LIB NAMES ${libname})
> + list(APPEND EXPORT_LIST ${SYMBOLS_LIB})
> + set(SYMBOLS_LIB "SYMBOLS_LIB-NOTFOUND")
Why do you set SYMBOLS_LIB manually?
> + else()
> + message(WARNING "${libstatic} should be a static")
> + endif()
> + endforeach(libstatic)
> + string(REPLACE ";" " " EXPORT_LIST "${EXPORT_LIST}")
> +endif()
> +
We discussed it verbally, but I think this way to export symbols from
libraries we depend need to be commented as well as motivation of
exporting.
> # Exports syntax is toolchain-dependent, preprocessing is necessary
> set(exports_file ${CMAKE_BINARY_DIR}/extra/exports.${CMAKE_SYSTEM_NAME})
> add_custom_target(preprocess_exports
> @@ -264,12 +296,14 @@ add_custom_command(
> DEPENDS ${CMAKE_SOURCE_DIR}/extra/exports
> COMMAND ${CMAKE_SOURCE_DIR}/extra/mkexports
> ${CMAKE_SOURCE_DIR}/extra/exports
> - ${exports_file} ${CMAKE_SYSTEM_NAME})
> + ${exports_file} ${CMAKE_SYSTEM_NAME}
> + ${EXPORT_LIST}
> +)
>
> add_executable(
> - tarantool main.cc
> - ${LIBUTIL_FREEBSD_SRC}/flopen.c
> - ${LIBUTIL_FREEBSD_SRC}/pidfile.c)
> + tarantool main.cc
> + ${LIBUTIL_FREEBSD_SRC}/flopen.c
> + ${LIBUTIL_FREEBSD_SRC}/pidfile.c)
>
> add_dependencies(tarantool build_bundled_libs preprocess_exports)
>
> @@ -294,7 +328,9 @@ if (TARGET_OS_DARWIN)
> set_target_properties(tarantool PROPERTIES
> LINK_FLAGS "-Wl,-exported_symbols_list,${exports_file}")
> else ()
> + message(${reexport_libraries} ${common_libraries})
Forgotten debug print?
> target_link_libraries(tarantool
> + ${generic_libraries}
Indent is strange here.
> -Wl,--whole-archive box ${reexport_libraries}
> salad -Wl,--no-whole-archive
> ${common_libraries})
> diff --git a/test/app-tap/suite.ini b/test/app-tap/suite.ini
> index 86af82637..85ad9174d 100644
> --- a/test/app-tap/suite.ini
> +++ b/test/app-tap/suite.ini
> @@ -3,3 +3,4 @@ core = app
> description = application server tests (TAP)
> lua_libs = lua/require_mod.lua lua/serializer_test.lua
> is_parallel = True
> +disabled = http_client.test.lua
Why it is part of this patch? There are no any mention of a reason in
the commit message.
> diff --git a/third_party/crc32.c b/third_party/crc32.c
> index a271d07ec..16b8c4040 100644
> --- a/third_party/crc32.c
> +++ b/third_party/crc32.c
> @@ -45,7 +45,7 @@
> #include <unistd.h>
> #include <stdint.h>
>
> -uint32_t crc32_table[] = {
> +static uint32_t crc32_table[] = {
> 0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,
> 0xe963a535, 0x9e6495a3, 0x0edb8832, 0x79dcb8a4, 0xe0d5e91e, 0x97d2d988,
> 0x09b64c2b, 0x7eb17cbd, 0xe7b82d07, 0x90bf1d91, 0x1db71064, 0x6ab020f2,
> @@ -92,7 +92,7 @@ uint32_t crc32_table[] = {
> };
>
> uint32_t
> -crc32(const void *buf, size_t size)
> +tnt_crc32(const void *buf, size_t size)
> {
> const uint8_t *p = buf;
> uint32_t crc;
> @@ -735,7 +735,7 @@ multitable_crc32c(uint32_t crc32c,
> }
>
> uint32_t
> -crc32c(uint32_t crc32c,
> +tnt_crc32c(uint32_t crc32c,
> const char *buffer,
> unsigned int length)
> {
> diff --git a/third_party/crc32.h b/third_party/crc32.h
> index 55c7072f3..8dc61d05d 100644
> --- a/third_party/crc32.h
> +++ b/third_party/crc32.h
> @@ -3,7 +3,9 @@
>
> #include <stdint.h>
>
> -uint32_t crc32(const void *buf, size_t size);
> -uint32_t crc32c(uint32_t crc32c, const char *buffer, unsigned int length);
> +#define crc32 tnt_crc32
> +uint32_t tnt_crc32(const void *buf, size_t size);
> +#define crc32c tnt_crc32c
> +uint32_t tnt_crc32c(uint32_t crc32c, const char *buffer, unsigned int length);
>
> #endif
Some conflict that are not mentioned in the commit message?
> --
> 2.18.0
>
>
More information about the Tarantool-patches
mailing list