Re: [Tarantool-patches] [PATCH 1/2] build: enable smtp

[Alexander Turenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Aside of several stylistic nits, I have no objections.

LGTM after changes (no need to re-review with me.)

Please, update and proceed with the next reviewer.

On Fri, Mar 19, 2021 at 04:45:54PM +0300, Roman Khabibov wrote:
> Enable smtp protocol in bundled libCURL.

Nit: Regarding 'libCURL'. The project page ([1]) name the library
'libcurl', so it would be better to follow it.

[1]: https://curl.se/libcurl/

Please, leave a couple of words why the change is needed and link the
relevant issue. I see that
https://github.com/tarantool/tarantool/issues/4559 is quite relevant.

> +## feature/build
> +
> +* Enable smtp protocol in bundled libCURL (gh-####).

Nit: 'gh-####' have no any sense. It is better to replace it with gh-4559.

> \ No newline at end of file

No newline at end of file.

> diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake
> index 39a1d62fd..59f78d9e2 100644
> --- a/cmake/BuildLibCURL.cmake
> +++ b/cmake/BuildLibCURL.cmake
> @@ -77,7 +77,7 @@ macro(curl_build)
>  
>      # Switch off the group of protocols with special flag HTTP_ONLY:
>      #   ftp, file, ldap, ldaps, rtsp, dict, telnet, tftp, pop3, imap, smtp.
> -    list(APPEND LIBCURL_CMAKE_FLAGS "-DHTTP_ONLY=ON")
> +    list(APPEND LIBCURL_CMAKE_FLAGS "-DHTTP_ONLY=OFF")
>  
>      # Additionaly disable some more protocols.
>      list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_SMB=ON")
> @@ -142,7 +142,7 @@ macro(curl_build)
>      list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_TFTP=ON")
>      list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_POP3=ON")
>      list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_IMAP=ON")
> -    list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_SMTP=ON")
> +    list(APPEND LIBCURL_CMAKE_FLAGS "-DCURL_DISABLE_SMTP=OFF")

I made the following checks:

1. Compared build/curl/work/curl/CMakeCache.txt before and after the
   change. Only given options are changed. Okay.

2. Added the following hunk to test/box-tap/curl-build.test.lua:

    | local i = 0
    | local protocol = info.protocols[i]
    | while protocol ~= nil do
    |     print(('protocols[%d] = %s'):format(i, ffi.string(protocol)))
    |     i = i + 1
    |     protocol = info.protocols[i]
    | end

   Before the patch:

    | protocols[0] = http
    | protocols[1] = https

   After the patch:

    | protocols[0] = http
    | protocols[1] = https
    | protocols[2] = smtp
    | protocols[3] = smtps

   (See, it worth to check for smtps as well.)

   The output looks okay.

3. Added the following hunk to test/box-tap/curl-build.test.lua:

    | print(('features: %d'):format(info.features))

   The output before and after the patch is the same:

    | features: 2622093

   Looks okay.

So, it seems, we don't enable or disable something unintentionally.
Nice.

> +--
> +-- gh-####: Check if smtp protocol is enabled.
> +--

Nit: Please, replace with gh-4559.

> +local function has_smtp()
> +    local i = 0
> +    -- curl_version_info_data.protocols is a null terminated array
> +    -- of pointers to char.
> +    -- See curl/lib/version.c:
> +    --   static const char * const protocols[]
> +    local protocol = info.protocols[i]
> +    while protocol ~= nil do
> +        if ffi.string(protocol) == 'smtp' then
> +            return true
> +        end

It worth to check for 'smtps' as well (see above).