From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 77470469719 for ; Wed, 14 Oct 2020 12:12:22 +0300 (MSK) Date: Wed, 14 Oct 2020 12:12:20 +0300 From: "Alexander V. Tikhonov" Message-ID: <20201014091220.GC1134582@hpalx> References: <4fc536691abbbbb9f3439bb60bef7a551663a5b5.1602143974.git.avtikhon@tarantool.org> <20201012185234.GA18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201012185234.GA18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3] build: enable cmake in curl build List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi Igor, I've made all of your suggestions except > > + # switch off the group of protocols with special flag HTTP_ONLY: > > + # ftp, file, ldap, ldaps, rtsp, dict, telnet, tftp, pop3, imap, smtp > > I guess this comment is irrelevant considering the options you disabled > manually below. where I think is correct and should stay for information. Also I still working on finding solution not to use hardcoded parallel build with: make -j On Mon, Oct 12, 2020 at 09:52:34PM +0300, Igor Munkin wrote: > Sasha, > > Thanks for the patch! > > On 08.10.20, Alexander V. Tikhonov wrote: > > Initially tryed to change autoconf tools in Curl build to cmake and > > Typo (again): s/tryed/tried/. > > > found the following build issue on: > > CentOS 6,7 > > Minor: IMHO, it's better to split these lines as you did in the commit > you're referring below. > > > Debian 8 > > Ubuntu 14.04 > > > > Issue found: > > CMake Error at CMakeLists.txt:41 (cmake_minimum_required): > > CMake 3.0 or higher is required. You are running version 2.8.12.2 > > > > To fix the issue removed the check of the version from curl sources > > Typo: s/removed the check/check is removed/. > > > in Tarantool's third party '/third_party/curl': > > > > 12af024bc85606b14ffc415413a7e86e6bbee7eb "Enable curl build with old cmake" > > Minor: We mention commits in the form > 0000000000000000000000000000000000000000 ('commit message header'): > full 40-digits hash and the commit message header in parenthesis. > > > > > After this fix completely changed autoconf to cmake in curl build. > > Autoconf part completely removed and code cleaned up for cmake. > > Typo: s/removed/is removed/. > > > For curl cmake build all autoconf options were ported to cmake > > configuration call, check the accurate list of the change in [1]. > > > > Also the following issues resolved: > > > > > > > 3. Found issues with building Tarantool statically on Debian 9 and > > its package build on CentOS 8. > > > > Building statically got the issues with openssl linking, like [2]: > > > > static-build/openssl-prefix/lib/libcrypto.a(threads_pthread.o): In function `CRYPTO_THREAD_lock_new': > > threads_pthread.c:(.text+0x45): undefined reference to `pthread_rwlock_init' > > > > It happened because in openssl radically changed how threads-related > > things were handled before 1.1.0 version. It required the application > > to provide us with the locking functionality in form of callbacks. > > Since 1.1.0, these matters are entirely internal, so libcrypto > > requires the presence of the platform specific thread implementation > > of our choosing, which is pthread on everything. > > > > After '-pthread' added to C compile flags package build on CentOS 8 > > failed with the issue [3]: > > > > /build/usr/src/debug/tarantool-2.6.0.141/third_party/curl/lib/warnless.c:101:4: error: #error "SIZEOF_CURL_OFF_T not defined" > > # error "SIZEOF_CURL_OFF_T not defined" > > ^~~~~ > > /build/usr/src/debug/tarantool-2.6.0.141/third_party/curl/lib/warnless.c: In function 'curlx_uztoso': > > /build/usr/src/debug/tarantool-2.6.0.141/third_party/curl/lib/warnless.c:192:40: error: 'CURL_MASK_SCOFFT' undeclared (first use in this function); did you mean 'CURL_MASK_SINT'? > > return (curl_off_t)(uznum & (size_t) CURL_MASK_SCOFFT); > > ^~~~~~~~~~~~~~~~ > > CURL_MASK_SINT > > > > To avoid of the issue decided to use '-pthread' flag only for openssl > > when it had not this flag in openssl compilation: > > > > # check '-pthread' in openssl compile options > > execute_process(COMMAND openssl version -f > > OUTPUT_VARIABLE OPENSSL_COMPILE_OPTIONS) > > # add pthread library for openssl static library linking > > if(NOT OPENSSL_COMPILE_OPTIONS MATCHES ".* -pthread .*") > > list(APPEND LIBCURL_CMAKE_FLAGS "-DCMAKE_C_FLAGS=-pthread") > > endif() > > Minor: I doubt code duplication in sources and commit message makes the > changes clearer :) The description above is enough (except it lacks the > reason why CURL_MASK_SCOFFT is undeclared, but it looks like minor > details that can be changed in future or googled by inquisitive readers) > > > > > > > > > > Closes #4968 > > Closes #5396 > > Closes #5019 > > Minor (again): Please sort the issues above. > > > > > [1] - https://github.com/tarantool/tarantool/issues/4968#issue-617183031 > > [2] - https://gitlab.com/tarantool/tarantool/-/jobs/779176133#L6021 > > [3] - https://gitlab.com/tarantool/tarantool/-/jobs/778309145#L3060 > > --- > > > > > > Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4874-out-of-source-build-full-ci > > Issue: https://github.com/tarantool/tarantool/issues/4968 > > Issue: https://github.com/tarantool/tarantool/issues/5396 > > Issue: https://github.com/tarantool/tarantool/issues/5019 > > > > V3: Added explicitly all disabled flags. > > Fixed #5396. > > Removed fix for #5020 as not needed now. > > > > cmake/BuildLibCURL.cmake | 249 +++++++++++++++++++-------------------- > > 1 file changed, 118 insertions(+), 131 deletions(-) > > > > diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake > > index 86fec39e9..911e49f11 100644 > > --- a/cmake/BuildLibCURL.cmake > > +++ b/cmake/BuildLibCURL.cmake > > @@ -3,11 +3,12 @@ macro(curl_build) > > set(LIBCURL_SOURCE_DIR ${PROJECT_SOURCE_DIR}/third_party/curl) > > set(LIBCURL_BINARY_DIR ${PROJECT_BINARY_DIR}/build/curl/work) > > set(LIBCURL_INSTALL_DIR ${PROJECT_BINARY_DIR}/build/curl/dest) > > + set(LIBCURL_CMAKE_FLAGS "") > > > > message(STATUS "Looking for zlib") > > find_path(ZLIB_INCLUDE_DIR zlib.h) > > message(STATUS "Looking for zlib.h - ${ZLIB_INCLUDE_DIR}") > > - if (BUILD_STATIC) > > + if(BUILD_STATIC) > > Minor: This line changes nothing except the whitespace (it looks like > rebase conflict resolution). Such changes only enlarge the patch, so I > believe they can be dropped. However, I have no strict objections, > regarding this one, since it doesn't introduce new hunks (ergo the > changes are quite local). > > > set(LIBZ_LIB_NAME libz.a) > > else() > > set(LIBZ_LIB_NAME z) > > @@ -15,44 +16,125 @@ macro(curl_build) > > find_library(LIBZ_LIBRARY NAMES ${LIBZ_LIB_NAME}) > > message(STATUS "Looking for libz - ${LIBZ_LIBRARY}") > > > > - if (NOT ZLIB_INCLUDE_DIR OR NOT LIBZ_LIBRARY) > > + if(NOT ZLIB_INCLUDE_DIR OR NOT LIBZ_LIBRARY) > > Ditto. > > > message(FATAL_ERROR "Unable to find zlib") > > endif() > > > > > + > > + # switch off the group of protocols with special flag HTTP_ONLY: > > + # ftp, file, ldap, ldaps, rtsp, dict, telnet, tftp, pop3, imap, smtp > > I guess this comment is irrelevant considering the options you disabled > manually below. > > > + list(APPEND LIBCURL_CMAKE_FLAGS "-DHTTP_ONLY=ON") > > > > > + # In hardened mode, which enables -fPIE by default, > > + # the cmake checks don't work without -fPIC. > > + list(APPEND LIBCURL_CMAKE_FLAGS "-DCMAKE_REQUIRED_FLAGS=-fPIC") > > As a result of the offline discussion this line is removed > > > + > > > > > > > include(ExternalProject) > > ExternalProject_Add( > > @@ -62,101 +144,11 @@ macro(curl_build) > > > > > - --disable-symbol-hiding > > There is option, that is ON (AFAIU, this > extension[1]). So, this change is inconsistent: you drop this configure > options and don't set the proper CMake flag. > > > > > + BUILD_COMMAND cd && $(MAKE) -j > > The comment regarding this line is left unaddressed. > > > INSTALL_COMMAND cd && $(MAKE) install) > > > > > -- > > 2.25.1 > > > > [1]: https://github.com/tarantool/curl/blob/5a1fc8d33808d7b22f57bdf9403cda7ff07b0670/CMake/CurlSymbolHiding.cmake#L24 > > -- > Best regards, > IM