[Tarantool-patches] [PATCH 0/2] Install curl headers and enable smtp

[Tarantool-patches] [PATCH 0/2] Install curl headers and enable smtp

[Roman Khabibov via Tarantool-patches]

The patchset is only for 2.8.

Issue: https://github.com/tarantool/smtp/issues/24
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/curl-smtp

Roman Khabibov (2):
  build: enable smtp
  build: install libCURL headers

 CMakeLists.txt                                |  7 +++++
 changelogs/unreleased/enable-smtp.md          |  3 ++
 changelogs/unreleased/install-headers.md      |  4 +++
 cmake/BuildLibCURL.cmake                      |  4 +--
 rpm/tarantool.spec                            |  9 ++++++
 ...l-exports.test.lua => curl-build.test.lua} | 28 ++++++++++++++++++-
 6 files changed, 52 insertions(+), 3 deletions(-)
 create mode 100755 changelogs/unreleased/enable-smtp.md
 create mode 100755 changelogs/unreleased/install-headers.md
 rename test/box-tap/{gh-5223-curl-exports.test.lua => curl-build.test.lua} (90%)

-- 
2.24.3 (Apple Git-128)

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

[Roman Khabibov via Tarantool-patches]

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-####).
\ 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
 
+--
+-- 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)
-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH 2/2] build: install libCURL headers

[Roman Khabibov via Tarantool-patches]

Ship libCURL headers to system path "include/tarantool" in the
case of libCURL included as bundled library or static build.
---
 CMakeLists.txt                           | 7 +++++++
 changelogs/unreleased/install-headers.md | 4 ++++
 rpm/tarantool.spec                       | 9 +++++++++
 3 files changed, 20 insertions(+)
 create mode 100755 changelogs/unreleased/install-headers.md

diff --git a/CMakeLists.txt b/CMakeLists.txt
index feb56dfca..005fe0cd7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -428,6 +428,13 @@ else()
     find_package(CURL)
 endif()
 
+# Install headers.
+if (ENABLE_BUNDLED_LIBCURL OR BUILD_STATIC)
+    install(DIRECTORY "${CURL_INCLUDE_DIRS}/curl"
+            DESTINATION ${MODULE_FULL_INCLUDEDIR}
+            FILES_MATCHING PATTERN "*.h")
+endif()
+
 #
 # Export libcurl symbols if the library is linked statically.
 #
diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
new file mode 100755
index 000000000..4494a14c8
--- /dev/null
+++ b/changelogs/unreleased/install-headers.md
@@ -0,0 +1,4 @@
+## feature/build
+
+* Ship libCURL headers to system path "include/tarantool" in the
+case of libCURL included as bundled library or static build (gh-####).
\ No newline at end of file
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 92e693955..d19df8925 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -268,6 +268,15 @@ fi
 %{_includedir}/tarantool/luajit.h
 %{_includedir}/tarantool/lualib.h
 %{_includedir}/tarantool/module.h
+%{_includedir}/tarantool/curl/curl.h
+%{_includedir}/tarantool/curl/curlver.h
+%{_includedir}/tarantool/curl/easy.h
+%{_includedir}/tarantool/curl/mprintf.h
+%{_includedir}/tarantool/curl/multi.h
+%{_includedir}/tarantool/curl/stdcheaders.h
+%{_includedir}/tarantool/curl/system.h
+%{_includedir}/tarantool/curl/typecheck-gcc.h
+%{_includedir}/tarantool/curl/urlapi.h
 
 %changelog
 * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
-- 
2.24.3 (Apple Git-128)

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

[Alexander Turenko via Tarantool-patches]

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).

Re: [Tarantool-patches] [PATCH 2/2] build: install libCURL headers

[Alexander Turenko via Tarantool-patches]

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

Please, update and proceed with the next reviewer.

On Fri, Mar 19, 2021 at 04:45:55PM +0300, Roman Khabibov wrote:
> Ship libCURL headers to system path "include/tarantool" in the
> case of libCURL included as bundled library or static build.

Please, reflect comments to the first patch here: the library naming,
motivation of the change, the issue number.

Nit: I suggest to refer include directory as
"${PREFIX}/include/tarantool" -- it makes quite clear that it may be
/usr/include/tarantool, /usr/local/include/tarantool or something of
this kind.

> diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
> new file mode 100755
> index 000000000..4494a14c8
> --- /dev/null
> +++ b/changelogs/unreleased/install-headers.md
> @@ -0,0 +1,4 @@
> +## feature/build
> +
> +* Ship libCURL headers to system path "include/tarantool" in the
> +case of libCURL included as bundled library or static build (gh-####).
> \ No newline at end of file

No newline at end of file.

> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index 92e693955..d19df8925 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -268,6 +268,15 @@ fi
>  %{_includedir}/tarantool/luajit.h
>  %{_includedir}/tarantool/lualib.h
>  %{_includedir}/tarantool/module.h
> +%{_includedir}/tarantool/curl/curl.h
> +%{_includedir}/tarantool/curl/curlver.h
> +%{_includedir}/tarantool/curl/easy.h
> +%{_includedir}/tarantool/curl/mprintf.h
> +%{_includedir}/tarantool/curl/multi.h
> +%{_includedir}/tarantool/curl/stdcheaders.h
> +%{_includedir}/tarantool/curl/system.h
> +%{_includedir}/tarantool/curl/typecheck-gcc.h
> +%{_includedir}/tarantool/curl/urlapi.h

AFAIR, just %{_includedir}/tarantool/curl (without %dir) should work
well and should install the whole directory. It'll allow us to update
libcurl beyond 7.73.0 (see [1]) without a fear to forget to update those
rules.

[1]: https://github.com/curl/curl/commit/6ebe63fac23f38df911edc348e8ccc72280f9434

There is a risk to miss a problem with installing the headers (if cmake
does not install it because of some problem), however partial installing
looks even worse.

How about Debian based distributions? Nothing to change, the headers
will be installed?

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

[Roman Khabibov via Tarantool-patches]

Hi! Thanks for the review.

> On Mar 31, 2021, at 02:13, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> 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.
Done.

> [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.
Added.

>> 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).

+local function has_protocol(protocol_str)
+    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 info = ffi.C.curl_version_info(7)
+    local protocol = info.protocols[i]
+    while protocol ~= nil do
+        if ffi.string(protocol) == protocol_str then
+            return true
+        end
+        i = i + 1
+        protocol = info.protocols[i]
+    end
+
+    return false
+end
+
+--
+-- gh-4559: check if smtp and smtps protocols are enabled.
+--
+test:ok(has_protocol('smtp'), 'smtp protocol is supported')
+test:ok(has_protocol('smtps'), 'smtps protocol is supported’)
+

commit 4d939d2ac120774af29f2dc1fd5dd1528f82fce8
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Jan 19 00:45:32 2021 +0300

    build: enable smtp
    
    Enable smtp and smtps protocols in bundled libcurl.
    
    Part of #4559

diff --git a/changelogs/unreleased/enable-smtp.md b/changelogs/unreleased/enable-smtp.md
new file mode 100755
index 000000000..aa299b3c2
--- /dev/null
+++ b/changelogs/unreleased/enable-smtp.md
@@ -0,0 +1,3 @@
+## feature/build
+
+* Enable smtp and smtps protocols in bundled libcurl (gh-4559).
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 88%
rename from test/box-tap/gh-5223-curl-exports.test.lua
rename to test/box-tap/curl-build.test.lua
index 300d60b07..ae0d397b5 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(5)
 
 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
 
+--
+-- 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,29 @@ test:test('curl_symbols', function(t)
     end
 end)
 
+local function has_protocol(protocol_str)
+    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 info = ffi.C.curl_version_info(7)
+    local protocol = info.protocols[i]
+    while protocol ~= nil do
+        if ffi.string(protocol) == protocol_str then
+            return true
+        end
+        i = i + 1
+        protocol = info.protocols[i]
+    end
+
+    return false
+end
+
+--
+-- gh-4559: check if smtp and smtps protocols are enabled.
+--
+test:ok(has_protocol('smtp'), 'smtp protocol is supported')
+test:ok(has_protocol('smtps'), 'smtps protocol is supported')
+
 os.exit(test:check() and 0 or 1)

Re: [Tarantool-patches] [PATCH 2/2] build: install libCURL headers

[Roman Khabibov via Tarantool-patches]

Hi! Thanks for the review.

> On Mar 31, 2021, at 02:14, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> LGTM after fixes (no need to re-review with me).
> 
> Please, update and proceed with the next reviewer.
> 
> On Fri, Mar 19, 2021 at 04:45:55PM +0300, Roman Khabibov wrote:
>> Ship libCURL headers to system path "include/tarantool" in the
>> case of libCURL included as bundled library or static build.
> 
> Please, reflect comments to the first patch here: the library naming,
> motivation of the change, the issue number.
> 
> Nit: I suggest to refer include directory as
> "${PREFIX}/include/tarantool" -- it makes quite clear that it may be
> /usr/include/tarantool, /usr/local/include/tarantool or something of
> this kind.
Done.

>> diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
>> new file mode 100755
>> index 000000000..4494a14c8
>> --- /dev/null
>> +++ b/changelogs/unreleased/install-headers.md
>> @@ -0,0 +1,4 @@
>> +## feature/build
>> +
>> +* Ship libCURL headers to system path "include/tarantool" in the
>> +case of libCURL included as bundled library or static build (gh-####).
>> \ No newline at end of file
> 
> No newline at end of file.
Added.

>> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
>> index 92e693955..d19df8925 100644
>> --- a/rpm/tarantool.spec
>> +++ b/rpm/tarantool.spec
>> @@ -268,6 +268,15 @@ fi
>> %{_includedir}/tarantool/luajit.h
>> %{_includedir}/tarantool/lualib.h
>> %{_includedir}/tarantool/module.h
>> +%{_includedir}/tarantool/curl/curl.h
>> +%{_includedir}/tarantool/curl/curlver.h
>> +%{_includedir}/tarantool/curl/easy.h
>> +%{_includedir}/tarantool/curl/mprintf.h
>> +%{_includedir}/tarantool/curl/multi.h
>> +%{_includedir}/tarantool/curl/stdcheaders.h
>> +%{_includedir}/tarantool/curl/system.h
>> +%{_includedir}/tarantool/curl/typecheck-gcc.h
>> +%{_includedir}/tarantool/curl/urlapi.h
> 
> AFAIR, just %{_includedir}/tarantool/curl (without %dir) should work
> well and should install the whole directory. It'll allow us to update
> libcurl beyond 7.73.0 (see [1]) without a fear to forget to update those
> rules.
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 92e693955..f8f6c124a 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -268,6 +268,7 @@ fi
 %{_includedir}/tarantool/luajit.h
 %{_includedir}/tarantool/lualib.h
 %{_includedir}/tarantool/module.h
+%{_includedir}/tarantool/curl

> [1]: https://github.com/curl/curl/commit/6ebe63fac23f38df911edc348e8ccc72280f9434
> 
> There is a risk to miss a problem with installing the headers (if cmake
> does not install it because of some problem), however partial installing
> looks even worse.
> 
> How about Debian based distributions? Nothing to change, the headers
> will be installed?

commit bec7a26a238dd1c03672d0f46ca082497a3027a0
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Sun Dec 20 13:08:40 2020 +0500

    build: install libcurl headers
    
    Ship libcurl headers to system path "${PREFIX}/include/tarantool"
    in the case of libcurl included as bundled library.
    
    Closes #4559

diff --git a/CMakeLists.txt b/CMakeLists.txt
index feb56dfca..1196b65b9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -428,6 +428,13 @@ else()
     find_package(CURL)
 endif()
 
+# Install headers.
+if (ENABLE_BUNDLED_LIBCURL)
+    install(DIRECTORY "${CURL_INCLUDE_DIRS}/curl"
+            DESTINATION ${MODULE_FULL_INCLUDEDIR}
+            FILES_MATCHING PATTERN "*.h")
+endif()
+
 #
 # Export libcurl symbols if the library is linked statically.
 #
diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
new file mode 100755
index 000000000..86363adb3
--- /dev/null
+++ b/changelogs/unreleased/install-headers.md
@@ -0,0 +1,4 @@
+## feature/build
+
+* Ship libcurl headers to system path "include/tarantool" in the
+case of libcurl included as bundled library or static build (gh-4559).
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 92e693955..f8f6c124a 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -268,6 +268,7 @@ fi
 %{_includedir}/tarantool/luajit.h
 %{_includedir}/tarantool/lualib.h
 %{_includedir}/tarantool/module.h
+%{_includedir}/tarantool/curl
 
 %changelog
 * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1

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

[Leonid Vasiliev via Tarantool-patches]

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)
> 

Re: [Tarantool-patches] [PATCH 2/2] build: install libCURL headers

[Leonid Vasiliev via Tarantool-patches]

Hi! Thank you for the patch.
Have you check that patch works fine on all distributions?
If yes - LGTM

On 4/9/21 10:55 PM, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
>> On Mar 31, 2021, at 02:14, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
>>
>> LGTM after fixes (no need to re-review with me).
>>
>> Please, update and proceed with the next reviewer.
>>
>> On Fri, Mar 19, 2021 at 04:45:55PM +0300, Roman Khabibov wrote:
>>> Ship libCURL headers to system path "include/tarantool" in the
>>> case of libCURL included as bundled library or static build.
>>
>> Please, reflect comments to the first patch here: the library naming,
>> motivation of the change, the issue number.
>>
>> Nit: I suggest to refer include directory as
>> "${PREFIX}/include/tarantool" -- it makes quite clear that it may be
>> /usr/include/tarantool, /usr/local/include/tarantool or something of
>> this kind.
> Done.
> 
>>> diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
>>> new file mode 100755
>>> index 000000000..4494a14c8
>>> --- /dev/null
>>> +++ b/changelogs/unreleased/install-headers.md
>>> @@ -0,0 +1,4 @@
>>> +## feature/build
>>> +
>>> +* Ship libCURL headers to system path "include/tarantool" in the
>>> +case of libCURL included as bundled library or static build (gh-####).
>>> \ No newline at end of file
>>
>> No newline at end of file.
> Added.
> 
>>> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
>>> index 92e693955..d19df8925 100644
>>> --- a/rpm/tarantool.spec
>>> +++ b/rpm/tarantool.spec
>>> @@ -268,6 +268,15 @@ fi
>>> %{_includedir}/tarantool/luajit.h
>>> %{_includedir}/tarantool/lualib.h
>>> %{_includedir}/tarantool/module.h
>>> +%{_includedir}/tarantool/curl/curl.h
>>> +%{_includedir}/tarantool/curl/curlver.h
>>> +%{_includedir}/tarantool/curl/easy.h
>>> +%{_includedir}/tarantool/curl/mprintf.h
>>> +%{_includedir}/tarantool/curl/multi.h
>>> +%{_includedir}/tarantool/curl/stdcheaders.h
>>> +%{_includedir}/tarantool/curl/system.h
>>> +%{_includedir}/tarantool/curl/typecheck-gcc.h
>>> +%{_includedir}/tarantool/curl/urlapi.h
>>
>> AFAIR, just %{_includedir}/tarantool/curl (without %dir) should work
>> well and should install the whole directory. It'll allow us to update
>> libcurl beyond 7.73.0 (see [1]) without a fear to forget to update those
>> rules.
> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index 92e693955..f8f6c124a 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -268,6 +268,7 @@ fi
>   %{_includedir}/tarantool/luajit.h
>   %{_includedir}/tarantool/lualib.h
>   %{_includedir}/tarantool/module.h
> +%{_includedir}/tarantool/curl
> 
>> [1]: https://github.com/curl/curl/commit/6ebe63fac23f38df911edc348e8ccc72280f9434
>>
>> There is a risk to miss a problem with installing the headers (if cmake
>> does not install it because of some problem), however partial installing
>> looks even worse.
>>
>> How about Debian based distributions? Nothing to change, the headers
>> will be installed?
> 
> commit bec7a26a238dd1c03672d0f46ca082497a3027a0
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Sun Dec 20 13:08:40 2020 +0500
> 
>      build: install libcurl headers
>      
>      Ship libcurl headers to system path "${PREFIX}/include/tarantool"
>      in the case of libcurl included as bundled library.
>      
>      Closes #4559
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index feb56dfca..1196b65b9 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -428,6 +428,13 @@ else()
>       find_package(CURL)
>   endif()
>   
> +# Install headers.
> +if (ENABLE_BUNDLED_LIBCURL)
> +    install(DIRECTORY "${CURL_INCLUDE_DIRS}/curl"
> +            DESTINATION ${MODULE_FULL_INCLUDEDIR}
> +            FILES_MATCHING PATTERN "*.h")
> +endif()
> +
>   #
>   # Export libcurl symbols if the library is linked statically.
>   #
> diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
> new file mode 100755
> index 000000000..86363adb3
> --- /dev/null
> +++ b/changelogs/unreleased/install-headers.md
> @@ -0,0 +1,4 @@
> +## feature/build
> +
> +* Ship libcurl headers to system path "include/tarantool" in the
> +case of libcurl included as bundled library or static build (gh-4559).
> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index 92e693955..f8f6c124a 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -268,6 +268,7 @@ fi
>   %{_includedir}/tarantool/luajit.h
>   %{_includedir}/tarantool/lualib.h
>   %{_includedir}/tarantool/module.h
> +%{_includedir}/tarantool/curl
>   
>   %changelog
>   * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
> 
> 

Re: [Tarantool-patches] [PATCH 0/2] Install curl headers and enable smtp

[Alexander Turenko via Tarantool-patches]

On Fri, Mar 19, 2021 at 04:45:53PM +0300, Roman Khabibov wrote:
> The patchset is only for 2.8.
> 
> Issue: https://github.com/tarantool/smtp/issues/24
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/curl-smtp
> 
> Roman Khabibov (2):
>   build: enable smtp
>   build: install libCURL headers

Rebased on actual master and 1.10 and verified in CI for regressions.

Pushed to master, 2.7, 2.6, 1.10.

WBR, Alexander Turenko.

----

My fixups:

1. Markdown formatting.

diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
index 86363adb3..003c7566e 100755
--- a/changelogs/unreleased/install-headers.md
+++ b/changelogs/unreleased/install-headers.md
@@ -1,4 +1,4 @@
 ## feature/build

 * Ship libcurl headers to system path "include/tarantool" in the
-case of libcurl included as bundled library or static build (gh-4559).
+  case of libcurl included as bundled library or static build (gh-4559).

2. Clafiried the include directory path in the changelog entry.

diff --git a/changelogs/unreleased/install-headers.md b/changelogs/unreleased/install-headers.md
index 003c7566e..957574c05 100755
--- a/changelogs/unreleased/install-headers.md
+++ b/changelogs/unreleased/install-headers.md
@@ -1,4 +1,4 @@
 ## feature/build

-* Ship libcurl headers to system path "include/tarantool" in the
+* Ship libcurl headers to system path "${PREFIX}/include/tarantool" in the
   case of libcurl included as bundled library or static build (gh-4559).

----

On 1.10:

3. Linked the relevant issues in the changelog entry.

diff --git a/changelogs/unreleased/export-curl-symbols.md b/changelogs/unreleased/export-curl-symbols.md
index 3a8213b1c..ac07ccab9 100644
--- a/changelogs/unreleased/export-curl-symbols.md
+++ b/changelogs/unreleased/export-curl-symbols.md
@@ -1,3 +1,4 @@
 ## feature/build

-* libcurl symbols in the case of bundled libcurl are now exported (gh-####).
+* libcurl symbols in the case of bundled libcurl are now exported (gh-5223,
+  gh-5542).

4. The similar change for the commit message.

-This patch is just 1.10 adaptation of two original commits 29ec628
-47c19ee.
+This patch is just 1.10 adaptation of two original commits:
+
+* 29ec628918bd7279de08e47becc070420775c2eb ('Ensure all curl symbols are
+  exported')
+* 47c19eeb4f67cb7257ce32542443c09410b6e752 ('build: don't re-export
+  libcurl.so/dylib symbols')
+
+Fixes #5223
+Fixes #5542

4. Removed a period from the URL.

-See related issue: https://github.com/tarantool/smtp/issues/24.
+See related issue: https://github.com/tarantool/smtp/issues/24

5. Added the comment from master's patch.

diff --git a/test/box-tap/curl-build.test.lua b/test/box-tap/curl-build.test.lua
index 640d5d143..ae0d397b5 100755
--- a/test/box-tap/curl-build.test.lua
+++ b/test/box-tap/curl-build.test.lua
@@ -78,6 +78,9 @@ else
     RTLD_DEFAULT = ffi.cast("void *", 0LL)
 end

+--
+-- 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 = {

6. Adjusted a comment to master's wording.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 26807c678..80337ffa0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -358,7 +358,7 @@ if (ENABLE_BUNDLED_LIBCURL)
 endif()

 #
-# Export libcurl symbols if the library is bundled.
+# Export libcurl symbols if the library is linked statically.
 #
 if (ENABLE_BUNDLED_LIBCURL)
     set(EXPORT_LIBCURL_SYMBOLS ON)

7. Mentioned actual commit hashes in backports.

+(backported from commit 4bde1dbc0fddbff04889f4a483dd97083435dd3a)
+(backported from commit 38d0b0c1f62886f40db1279da64f2fac75473119)