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

["Alexander V. Tikhonov" <avtikhon@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 '<root Tarantool sources>/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:
> > 
> 
> <snipped>
> 
> > 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)
> 
> > 
> 
> <snipped>
> 
> > 
> > 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()
> 
> <snipped>
> 
> > +
> > +    # 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")
> 
> <snipped>
> 
> > +    # 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
> 
> > +
> 
> <snipped>
> 
> >  
> >      include(ExternalProject)
> >      ExternalProject_Add(
> > @@ -62,101 +144,11 @@ macro(curl_build)
> 
> <snipped>
> 
> > -                --disable-symbol-hiding
> 
> There is <CURL_HIDDEN_SYMBOLS> 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.
> 
> <snipped>
> 
> > +        BUILD_COMMAND cd <BINARY_DIR> && $(MAKE) -j
> 
> The comment regarding this line is left unaddressed.
> 
> >          INSTALL_COMMAND cd <BINARY_DIR> && $(MAKE) install)
> 
> <snipped>
> 
> > -- 
> > 2.25.1
> > 
> 
> [1]: https://github.com/tarantool/curl/blob/5a1fc8d33808d7b22f57bdf9403cda7ff07b0670/CMake/CurlSymbolHiding.cmake#L24
> 
> -- 
> Best regards,
> IM