From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 91E4C469719 for ; Wed, 16 Sep 2020 23:25:53 +0300 (MSK) Date: Wed, 16 Sep 2020 23:15:21 +0300 From: Igor Munkin Message-ID: <20200916201521.GM18920@tarantool.org> References: <2be47cd197cef211b74d24259140f899becc2089.1594373392.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2be47cd197cef211b74d24259140f899becc2089.1594373392.git.avtikhon@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] build: enable cmake in curl build List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko Sasha, Thanks for the patch! Please consider my comments below. On 10.07.20, Alexander V. Tikhonov wrote: > Tryed to chang autoconf tools in Curl build to cmake use for builds Typo: s/Tryed/Tried/. Typo: s/chang/change/. > where cmake major starts from 3. It was made so because cmake in curl > build works only with 3.0 version and above, the following issue > found on: > CentOS 6,7 > Ubuntu 14.04 > > Issue with curl cmake based build with cmake before 3.0 version: > CMake Error at CMakeLists.txt:41 (cmake_minimum_required): > CMake 3.0 or higher is required. You are running version 2.8.12.2 > > For curl cmake build all autoconf options were ported to cmake > configuration call. > > Also found that CURL cmake configuration file: > third_party/curl/lib/CMakeLists.txt > > has installation part for built libcurl.a library: > install(TARGETS ${LIB_NAME} > EXPORT ${TARGETS_EXPORT_NAME} > ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} > LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} > > where it changes CMAKE_INSTALL_LIBDIR to appropriate name with > suffix of the architecture, like: > lib > lib64 > x86_64 > > Found that find_library routine from the file: > cmake/FindLibCURL.cmake > returns only 'lib' value and it breaks the building of the depends > binaries. To avoid of it the CMAKE_INSTALL_LIBDIR option was set to > cmake call: > -DCMAKE_INSTALL_LIBDIR=lib > > Completely changed autoconf to cmake in curl build. After curl > sources were changed to be able to be build since 2.8 version > the old OS like CentOS 6/7 and Ubuntu 14.04 became available > for curl build using cmake. OK, so the issue with old CMake is gone, isn't it? Let's reword the part at the beginning: | Replaced autoconf tools in CURL build with CMake. Since vanilla CURL | CMakeLists.txt requires CMake 3.0 or higher, our fork has been patched | in c4c5dda4774a51670d6ea9857750c74816a14759 (build: accept cmake 2.8 | version for build) to support the distros providing the older CMake by | default: | * CentOS 6 | * CentOS 7 | * Ubuntu 14.04 | | All autoconf build options were respectively converted to CMake flags. > > Autoconf part completely removed and code cleaned up for cmake. > > 1. Found issue with building on CentOS 6: > > Linking C executable curl > build/ares/dest/lib/libcares.a(ares__timeval.c.o): In function `ares__tvnow': > ares__timeval.c:(.text+0x15): undefined reference to `clock_gettime' > collect2: error: ld returned 1 exit status > > It was fixed with added "-lrt" flag to CMAKE_C_FLAGS and > CMAKE_CXX_FLAGS build flags, when cmake version is lower > than 3.0 and RT library had needed function. > > 2. Found issue with building FreeBSD 12: app/socket test failed. > > [035] --- app/socket.result Tue May 26 03:06:32 2020 > [035] +++ app/socket.reject Fri May 8 08:26:16 2020 > [035] @@ -836,7 +836,7 @@ > [035] ... > [035] s:recv() > [035] --- > [035] -- Hello, world > [035] +- > [035] ... > [035] sc:sendto('127.0.0.1', s:name().port, 'Hello, world, 2') > [035] --- > > It was fixed with added "-DLDFLAGS=" flag to cmake call. This define was set before, so it doesn't look like an issue (IMHO). > > 3. Found issue with static build using CentOS 7, where SSL cmake rule > failed. Building the image got the issue: > > [ 1%] Performing configure step for 'bundled-libcurl-project' > CMake Warning at CMakeLists.txt:50 (message): > the curl cmake build system is poorly maintained. Be aware > > -- curl version=[7.66.0-DEV] > -- Found c-ares: /tarantool/build/ares/dest/lib/libcares.a > Found *nroff option: -- -man > CMake Error at /usr/share/cmake/Modules/FindOpenSSL.cmake:278 (list): > list GET given empty list > Call Stack (most recent call first): > CMakeLists.txt:347 (find_package) > > Root cause of the issue that Dockerfile uses globaly > installed openSSL with: > > cmake ... -DOPENSSL_ROOT_DIR=/usr/local ... > > Its cmake build file: > > /usr/share/cmake/Modules/FindOpenSSL.cmake > > fails on parsing the SSL version: > > it has: > REGEX "^#define[\t ]+OPENSSL_VERSION_NUMBER[\t ]+0x([0-9a-fA-F])+.*") > > but it should to use: > REGEX "^#[\t ]*define[\t ]+OPENSSL_VERSION_NUMBER[\t ]+0x([0-9a-fA-F])+.*") > > Anyway we want to use the same OpenSSL library for libcurl, as is used > for Tarantool itself. So the path to it set for cURL build: > > list(APPEND LIBCURL_CMAKE_FLAGS "-DCMAKE_MODULE_PATH=${PROJECT_SOURCE_DIR}/cmake") > > Closes #5019 > Closes #4968 > Closes #5020 Minor: Please sort the issues above. > --- By the way, I don't see this commit on this branch: | commit c4c5dda4774a51670d6ea9857750c74816a14759 | Author: Alexander V. Tikhonov | Date: Sat May 23 13:55:04 2020 +0300 | | build: accept cmake 2.8 version for build | | Changed cmake minimal accepted version for curl build from 3.0 to 2.8. > > 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/5019 > Issue: https://github.com/tarantool/tarantool/issues/5020 > V2: moved fix for #5019 from standalone commit to current > > cmake/BuildLibCURL.cmake | 202 ++++++++++++++------------------------- > 1 file changed, 74 insertions(+), 128 deletions(-) > > diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake > index 5f8b15a63..022dd90cd 100644 > --- a/cmake/BuildLibCURL.cmake > +++ b/cmake/BuildLibCURL.cmake > @@ -14,39 +15,80 @@ macro(curl_build) > message(FATAL_ERROR "Unable to find zlib") > endif() > > - # Use the same OpenSSL library for libcurl as is used for > - # tarantool itself. > + # add librt for clock_gettime function definition > + if(${CMAKE_MAJOR_VERSION} VERSION_LESS "3") > + CHECK_LIBRARY_EXISTS (rt clock_gettime "" HAVE_LIBRT) > + if (HAVE_LIBRT) Typo: whitespace differs from the code nearby. > + list(APPEND LIBCURL_CMAKE_FLAGS "-DCMAKE_CXX_FLAGS=-lrt") > + list(APPEND LIBCURL_CMAKE_FLAGS "-DCMAKE_C_FLAGS=-lrt") > + endif() > + endif() > + > + # switch off the shared build Minor: Build for shared libs was enabled in autoconf version. If you disabled it intentionally, please mention it within commit message. > + list(APPEND LIBCURL_CMAKE_FLAGS "-DBUILD_SHARED_LIBS=OFF") > + # found that on FreeBSD 12 this setup is needed for app/socket.test.lua I see the related comment below. Does it make sense to leave it here? > + list(APPEND LIBCURL_CMAKE_FLAGS "-DLDFLAGS=") > + > + # 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") Sorry, but I don't get this line. What does it solve? > > include(ExternalProject) > ExternalProject_Add( > @@ -56,100 +98,11 @@ macro(curl_build) > DOWNLOAD_DIR ${LIBCURL_BINARY_DIR} > TMP_DIR ${LIBCURL_BINARY_DIR}/tmp > STAMP_DIR ${LIBCURL_BINARY_DIR}/stamp > - # Pass empty LDFLAGS to discard using of > - # corresponding environment variable. > - # It is possible that a linker flag assumes that > - # some compilation flag is set. We don't pass > - # CFLAGS from environment, so we should not do it > - # for LDFLAGS too. I guess this is a valuable comment to be saved in the patch. > - LDFLAGS= > - --without-brotli I totally agree with Sasha Tu.[1] regarding explicitly disabling the options that are disabled by default in CURL CMake. > - --without-gnutls > - BUILD_COMMAND cd && $(MAKE) > + cd && cmake > + ${LIBCURL_CMAKE_FLAGS} > + BUILD_COMMAND cd && $(MAKE) -j Minor: -j is OK, but it's better to use -j value from the parent scope. > INSTALL_COMMAND cd && $(MAKE) install) > -- > 2.17.1 > [1]: https://github.com/tarantool/tarantool/issues/4968#issuecomment-692420653 -- Best regards, IM