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

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

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