[Tarantool-patches] [PATCH] build: don't re-export libcurl.so/dylib symbols

[Tarantool-patches] [PATCH] build: don't re-export libcurl.so/dylib symbols

[Alexander Turenko]

Export libcurl's symbols only when they are provided by tarantool
itself: when the library is linked statically into the tarantool's
executable. There is no much sense to export the symbols when we link
against the library dynamically.

Regarding motivation of the change. Since 2.6.0-36-g29ec62891 ('Ensure
all curl symbols are exported') the curl_multi_poll() function is
exported from the tarantool executable. It leads to a failure in
Homebrew's build, because there we link (dynamically) with a system
libcurl. On Mac OS 10.15 it is libcurl 7.64.1, while the function
appears since libcurl 7.66.0. So a linker reports the undefined symbol:
`curl_multi_poll`.

Now the symbols are not exported at dynamic linking with libcurl, so the
linker is happy.

This commit relaxes bounds for dynamic linking, but an attempt to link
with libcurl older than 7.66.0 statically still leads to a linking
failure. The box-tap/gh-5223-curl-exports.test.lua test still fails when
tarantool is linked (dynamically) against an old libcurl.

It looks as the good compromise. When libcurl functionality is provided
by tarantool itself, *all* functions listed in the test are present
(otherwise a linker will complain). But tarantool does not enforce a
newer libcurl version, when it just *uses* this functionality and don't
provide it for modules and stored procedured. It is not tarantool's
responsibility in the case.

We possibly should skip the box-tap/gh-5223-curl-exports.test.lua test
when tarantool is built against libcurl dynamically or revisit the
described approach. I'll leave it as possible follow up activity.

Fixes #5542
---

https://github.com/tarantool/tarantool/issues/5542
https://github.com/tarantool/tarantool/tree/Totktonada/gh-5542-dont-export-libcurl-so-symbols
Totktonada/gh-5542-dont-export-libcurl-so-symbols

 CMakeLists.txt            | 10 ++++++++++
 src/exports.c             |  2 ++
 src/exports.h             |  2 ++
 src/trivia/config.h.cmake |  2 ++
 4 files changed, 16 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f0e1f5090..4fbd19558 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -397,6 +397,16 @@ else()
     find_package(CURL)
 endif()
 
+#
+# Export libcurl symbols if the library is linked statically.
+#
+if (ENABLE_BUNDLED_LIBCURL OR BUILD_STATIC)
+    set(EXPORT_LIBCURL_SYMBOLS ON)
+else()
+    set(EXPORT_LIBCURL_SYMBOLS OFF)
+endif()
+message(STATUS "EXPORT_LIBCURL_SYMBOLS: ${EXPORT_LIBCURL_SYMBOLS}")
+
 #
 # ReadLine
 #
diff --git a/src/exports.c b/src/exports.c
index a3c27143e..2a9c296a8 100644
--- a/src/exports.c
+++ b/src/exports.c
@@ -88,6 +88,8 @@
  * below. But automatically.
  */
 
+#include "trivia/config.h"
+
 /**
  * Symbol is just an address. No need to know its definition or
  * even type to get that address. Even an integer global variable
diff --git a/src/exports.h b/src/exports.h
index 2116036fa..1b95e75f6 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -130,6 +130,7 @@ EXPORT(csv_feed)
 EXPORT(csv_iterator_create)
 EXPORT(csv_next)
 EXPORT(csv_setopt)
+#ifdef EXPORT_LIBCURL_SYMBOLS
 EXPORT(curl_easy_cleanup)
 EXPORT(curl_easy_duphandle)
 EXPORT(curl_easy_escape)
@@ -211,6 +212,7 @@ EXPORT(curl_url_get)
 EXPORT(curl_url_set)
 EXPORT(curl_version)
 EXPORT(curl_version_info)
+#endif /* EXPORT_LIBCURL_SYMBOLS */
 EXPORT(decimal_unpack)
 EXPORT(error_ref)
 EXPORT(error_set_prev)
diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
index 21efb978e..38f8eb4dc 100644
--- a/src/trivia/config.h.cmake
+++ b/src/trivia/config.h.cmake
@@ -258,6 +258,8 @@
 
 #cmakedefine ENABLE_FEEDBACK_DAEMON 1
 
+#cmakedefine EXPORT_LIBCURL_SYMBOLS 1
+
 /*
  * vim: syntax=c
  */
-- 
2.25.0

Re: [Tarantool-patches] [PATCH] build: don't re-export libcurl.so/dylib symbols

[Alexander V. Tikhonov]

Hi Alexander, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/235694227

On Wed, Dec 30, 2020 at 07:53:42AM +0300, Alexander Turenko wrote:
> Export libcurl's symbols only when they are provided by tarantool
> itself: when the library is linked statically into the tarantool's
> executable. There is no much sense to export the symbols when we link
> against the library dynamically.
> 
> Regarding motivation of the change. Since 2.6.0-36-g29ec62891 ('Ensure
> all curl symbols are exported') the curl_multi_poll() function is
> exported from the tarantool executable. It leads to a failure in
> Homebrew's build, because there we link (dynamically) with a system
> libcurl. On Mac OS 10.15 it is libcurl 7.64.1, while the function
> appears since libcurl 7.66.0. So a linker reports the undefined symbol:
> `curl_multi_poll`.
> 
> Now the symbols are not exported at dynamic linking with libcurl, so the
> linker is happy.
> 
> This commit relaxes bounds for dynamic linking, but an attempt to link
> with libcurl older than 7.66.0 statically still leads to a linking
> failure. The box-tap/gh-5223-curl-exports.test.lua test still fails when
> tarantool is linked (dynamically) against an old libcurl.
> 
> It looks as the good compromise. When libcurl functionality is provided
> by tarantool itself, *all* functions listed in the test are present
> (otherwise a linker will complain). But tarantool does not enforce a
> newer libcurl version, when it just *uses* this functionality and don't
> provide it for modules and stored procedured. It is not tarantool's
> responsibility in the case.
> 
> We possibly should skip the box-tap/gh-5223-curl-exports.test.lua test
> when tarantool is built against libcurl dynamically or revisit the
> described approach. I'll leave it as possible follow up activity.
> 
> Fixes #5542
> ---
> 
> https://github.com/tarantool/tarantool/issues/5542
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-5542-dont-export-libcurl-so-symbols
> Totktonada/gh-5542-dont-export-libcurl-so-symbols
> 
>  CMakeLists.txt            | 10 ++++++++++
>  src/exports.c             |  2 ++
>  src/exports.h             |  2 ++
>  src/trivia/config.h.cmake |  2 ++
>  4 files changed, 16 insertions(+)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index f0e1f5090..4fbd19558 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -397,6 +397,16 @@ else()
>      find_package(CURL)
>  endif()
>  
> +#
> +# Export libcurl symbols if the library is linked statically.
> +#
> +if (ENABLE_BUNDLED_LIBCURL OR BUILD_STATIC)
> +    set(EXPORT_LIBCURL_SYMBOLS ON)
> +else()
> +    set(EXPORT_LIBCURL_SYMBOLS OFF)
> +endif()
> +message(STATUS "EXPORT_LIBCURL_SYMBOLS: ${EXPORT_LIBCURL_SYMBOLS}")
> +
>  #
>  # ReadLine
>  #
> diff --git a/src/exports.c b/src/exports.c
> index a3c27143e..2a9c296a8 100644
> --- a/src/exports.c
> +++ b/src/exports.c
> @@ -88,6 +88,8 @@
>   * below. But automatically.
>   */
>  
> +#include "trivia/config.h"
> +
>  /**
>   * Symbol is just an address. No need to know its definition or
>   * even type to get that address. Even an integer global variable
> diff --git a/src/exports.h b/src/exports.h
> index 2116036fa..1b95e75f6 100644
> --- a/src/exports.h
> +++ b/src/exports.h
> @@ -130,6 +130,7 @@ EXPORT(csv_feed)
>  EXPORT(csv_iterator_create)
>  EXPORT(csv_next)
>  EXPORT(csv_setopt)
> +#ifdef EXPORT_LIBCURL_SYMBOLS
>  EXPORT(curl_easy_cleanup)
>  EXPORT(curl_easy_duphandle)
>  EXPORT(curl_easy_escape)
> @@ -211,6 +212,7 @@ EXPORT(curl_url_get)
>  EXPORT(curl_url_set)
>  EXPORT(curl_version)
>  EXPORT(curl_version_info)
> +#endif /* EXPORT_LIBCURL_SYMBOLS */
>  EXPORT(decimal_unpack)
>  EXPORT(error_ref)
>  EXPORT(error_set_prev)
> diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
> index 21efb978e..38f8eb4dc 100644
> --- a/src/trivia/config.h.cmake
> +++ b/src/trivia/config.h.cmake
> @@ -258,6 +258,8 @@
>  
>  #cmakedefine ENABLE_FEEDBACK_DAEMON 1
>  
> +#cmakedefine EXPORT_LIBCURL_SYMBOLS 1
> +
>  /*
>   * vim: syntax=c
>   */
> -- 
> 2.25.0
> 

Re: [Tarantool-patches] [PATCH] build: don't re-export libcurl.so/dylib symbols

[Kirill Yukhin]

Hello,

On 30 Dec 07:53, Alexander Turenko wrote:
> Export libcurl's symbols only when they are provided by tarantool
> itself: when the library is linked statically into the tarantool's
> executable. There is no much sense to export the symbols when we link
> against the library dynamically.
> 
> Regarding motivation of the change. Since 2.6.0-36-g29ec62891 ('Ensure
> all curl symbols are exported') the curl_multi_poll() function is
> exported from the tarantool executable. It leads to a failure in
> Homebrew's build, because there we link (dynamically) with a system
> libcurl. On Mac OS 10.15 it is libcurl 7.64.1, while the function
> appears since libcurl 7.66.0. So a linker reports the undefined symbol:
> `curl_multi_poll`.
> 
> Now the symbols are not exported at dynamic linking with libcurl, so the
> linker is happy.
> 
> This commit relaxes bounds for dynamic linking, but an attempt to link
> with libcurl older than 7.66.0 statically still leads to a linking
> failure. The box-tap/gh-5223-curl-exports.test.lua test still fails when
> tarantool is linked (dynamically) against an old libcurl.
> 
> It looks as the good compromise. When libcurl functionality is provided
> by tarantool itself, *all* functions listed in the test are present
> (otherwise a linker will complain). But tarantool does not enforce a
> newer libcurl version, when it just *uses* this functionality and don't
> provide it for modules and stored procedured. It is not tarantool's
> responsibility in the case.
> 
> We possibly should skip the box-tap/gh-5223-curl-exports.test.lua test
> when tarantool is built against libcurl dynamically or revisit the
> described approach. I'll leave it as possible follow up activity.
> 
> Fixes #5542

LGTM.
I've checked your patch into 2.6 and master.

--
Regards, Kirill Yukhin