Re: [Tarantool-patches] [PATCH v2] build: enable cmake in curl build

[Igor Munkin <imun@tarantool.org>]

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 <avtikhon@tarantool.org>
| 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

<snipped>

> @@ -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()
> +

<snipped>

> +    # 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")

<snipped>

> +    # 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

<snipped>

> -                # 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=

<snipped>

> -                --without-brotli

I totally agree with Sasha Tu.[1] regarding explicitly disabling the
options that are disabled by default in CURL CMake.

> -                --without-gnutls

<snipped>

> -        BUILD_COMMAND cd <BINARY_DIR> && $(MAKE)
> +            cd <BINARY_DIR> && cmake <SOURCE_DIR>
> +                ${LIBCURL_CMAKE_FLAGS}
> +        BUILD_COMMAND cd <BINARY_DIR> && $(MAKE) -j

Minor: -j is OK, but it's better to use -j value from the parent scope.

>          INSTALL_COMMAND cd <BINARY_DIR> && $(MAKE) install)

<snipped>

> -- 
> 2.17.1
> 

[1]: https://github.com/tarantool/tarantool/issues/4968#issuecomment-692420653

-- 
Best regards,
IM