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

[Leonid Vasiliev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thank you for the patch.
Generally LGTM. See 2 comments below.

On 3/19/21 4:45 PM, Roman Khabibov via Tarantool-patches wrote:
> Enable smtp protocol in bundled libCURL.
> ---
>   changelogs/unreleased/enable-smtp.md          |  3 ++
>   cmake/BuildLibCURL.cmake                      |  4 +--
>   ...l-exports.test.lua => curl-build.test.lua} | 28 ++++++++++++++++++-
>   3 files changed, 32 insertions(+), 3 deletions(-)
>   create mode 100755 changelogs/unreleased/enable-smtp.md
>   rename test/box-tap/{gh-5223-curl-exports.test.lua => curl-build.test.lua} (90%)
> 
> diff --git a/changelogs/unreleased/enable-smtp.md b/changelogs/unreleased/enable-smtp.md
> new file mode 100755
> index 000000000..da5923e0a
> --- /dev/null
> +++ b/changelogs/unreleased/enable-smtp.md
> @@ -0,0 +1,3 @@
> +## feature/build
> +
> +* Enable smtp protocol in bundled libCURL (gh-####).

1) gh-####?
Here and below.

> \ 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")
>   
>       include(ExternalProject)
>       ExternalProject_Add(
> diff --git a/test/box-tap/gh-5223-curl-exports.test.lua b/test/box-tap/curl-build.test.lua
> similarity index 90%
> rename from test/box-tap/gh-5223-curl-exports.test.lua
> rename to test/box-tap/curl-build.test.lua
> index 300d60b07..f3041331b 100755
> --- a/test/box-tap/gh-5223-curl-exports.test.lua
> +++ b/test/box-tap/curl-build.test.lua
> @@ -56,7 +56,7 @@ ffi.cdef([[
>   
>   local info = ffi.C.curl_version_info(7)
>   local test = tap.test('curl-features')
> -test:plan(3)
> +test:plan(4)
>   
>   if test:ok(info.ssl_version ~= nil, 'Curl built with SSL support') then
>       test:diag('ssl_version: ' .. ffi.string(info.ssl_version))
> @@ -78,6 +78,9 @@ else
>       RTLD_DEFAULT = ffi.cast("void *", 0LL)
>   end
>   
> +--

2) Use "---" at the beginning of the function description.
Here and below.

> +-- gh-5223: Check if all curl symbols are exported.
> +--
>   -- The following list was obtained by parsing libcurl.a static library:
>   -- nm libcurl.a | grep -oP 'T \K(curl_.+)$' | sort
>   local curl_symbols = {
> @@ -174,4 +177,27 @@ test:test('curl_symbols', function(t)
>       end
>   end)
>   
> +--
> +-- gh-####: Check if smtp protocol is enabled.
> +--
> +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
> +        i = i + 1
> +        protocol = info.protocols[i]
> +    end
> +
> +    return false
> +end
> +
> +test:ok(has_smtp(), 'smtp protocol supported')
> +
>   os.exit(test:check() and 0 or 1)
>