[tarantool-patches] [PATCH] build: fix linking with static openssl library

[tarantool-patches] [PATCH] build: fix linking with static openssl library

[Alexander Turenko]

System-wide dynamic libraries usually (always?) have NEEDED and RUNPATH
tags in a dynamic section (as `readelf -d /usr/lib/lib<...>.so` shows),
so when we link, say, with libssl.so, which depends on libz.so, a linker
does not complain against unresolved symbols that can be found in Z
library (if it is installed within a system).

Things are different when we linking with a static library. Say, when we
linking with libssl.a, which contains an unresolved symbol from Z
library, a linker reports an error. It is not possible to store an
information where to find unresolved symbols (NEEDED / RUNPATH) in a
static library (AFAIK).

We depend on three libraries that are depend on Z library: libcurl,
libssl and libcrypto (two latter are part of OpenSSL). When one of those
libraries is linked statically we should link with libz.so or libz.a
(depending on BUILD_STATIC flag). The patch doing exactly this.

Before this patch we add Z library to OPENSSL_LIBRARIES when
BUILD_STATIC is enabled. It is not quite correct: we should do that when
OPENSSL_USE_STATIC_LIBS is enabled. However we didn't experienced
problems with static linking of libcurl (when BUILD_STATIC is enabled),
because OPENSSL_LIBRARIES is added to CURL_LIBRARIES. This is a kind of
side effect of dependency libcurl on OpenSSL and it is better to add
libz explicitly to CURL_LIBRARIES (and OPENSSL_LIBRARIES) when
appropriate.

Fixes #4437.
---

https://github.com/tarantool/tarantool/issues/4437
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4437-fix-static-openssl-build-full-ci

I more or less understood what is going on with all that linking stuff,
but it would be glad if you'll check my wording around this topic.

I doubt that the ability to link OpenSSL statically, but other libs
dynamically is really needed, but the commit at least holds the approach
when to add transitive dependencies to ours and when don't.

 CMakeLists.txt       | 10 +++++++---
 cmake/FindCURL.cmake |  6 ++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9b3950bdf..ddca8db85 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -324,10 +324,14 @@ endif()
 
 #
 # OpenSSL can require Z library (depending on build time options), so we add
-# it to libraries list in case of static build.
+# it to libraries list in case of static openssl linking.
 #
-if(BUILD_STATIC)
-    find_library(Z_LIBRARY libz.a)
+if(OPENSSL_USE_STATIC_LIBS)
+    if(BUILD_STATIC)
+        find_library(Z_LIBRARY libz.a)
+    else()
+        find_library(Z_LIBRARY z)
+    endif()
     set(OPENSSL_LIBRARIES ${OPENSSL_LIBRARIES} ${Z_LIBRARY})
 endif()
 
diff --git a/cmake/FindCURL.cmake b/cmake/FindCURL.cmake
index e13b6cde4..6193a58ad 100644
--- a/cmake/FindCURL.cmake
+++ b/cmake/FindCURL.cmake
@@ -18,9 +18,11 @@
 
 if(BUILD_STATIC)
     set(CURL_LIB_NAME libcurl.a)
+    set(Z_LIB_NAME libz.a)
     set(NGHTTP2_LIB_NAME libnghttp2.a)
 else()
     set(CURL_LIB_NAME curl)
+    set(Z_LIB_NAME z)
     set(NGHTTP2_LIB_NAME nghttp2)
 endif()
 
@@ -30,6 +32,7 @@ set(DL_LIB_NAME dl)
 
 # Curl may be linked with optional or target-dependent libraries,
 # search for them and add to dependicies if found.
+find_library(Z_LIBRARY NAMES ${Z_LIB_NAME})
 find_library(NGHTTP2_LIBRARY NAMES ${NGHTTP2_LIB_NAME})
 find_library(PTHREAD_LIBRARY NAMES ${PTHREAD_LIB_NAME})
 find_library(DL_LIBRARY NAMES ${DL_LIB_NAME})
@@ -88,6 +91,9 @@ if(CURL_FOUND)
   set(CURL_LIBRARIES ${CURL_LIBRARIES} ${OPENSSL_LIBRARIES})
   if(BUILD_STATIC)
     # In case of a static build we have to add curl dependencies.
+    if(NOT "${Z_LIBRARY}" STREQUAL "Z_LIBRARY-NOTFOUND")
+      set(CURL_LIBRARIES ${CURL_LIBRARIES} ${Z_LIBRARY})
+    endif()
     if(NOT "${NGHTTP2_LIBRARY}" STREQUAL "NGHTTP2_LIBRARY-NOTFOUND")
       set(CURL_LIBRARIES ${CURL_LIBRARIES} ${NGHTTP2_LIBRARY})
     endif()
-- 
2.22.0

[tarantool-patches] Re: [PATCH] build: fix linking with static openssl library

[Vladislav Shpilevoy]

Thanks for the patch!

On 23/08/2019 04:08, Alexander Turenko wrote:
> System-wide dynamic libraries usually (always?) have NEEDED and RUNPATH
> tags in a dynamic section (as `readelf -d /usr/lib/lib<...>.so` shows),
> so when we link, say, with libssl.so, which depends on libz.so, a linker
> does not complain against unresolved symbols that can be found in Z
> library (if it is installed within a system).
> 
> Things are different when we linking with a static library. Say, when we
> linking with libssl.a, which contains an unresolved symbol from Z
> library, a linker reports an error. It is not possible to store an
> information where to find unresolved symbols (NEEDED / RUNPATH) in a
> static library (AFAIK).
> 
> We depend on three libraries that are depend on Z library: libcurl,
> libssl and libcrypto (two latter are part of OpenSSL). When one of those
> libraries is linked statically we should link with libz.so or libz.a
> (depending on BUILD_STATIC flag). The patch doing exactly this.
> Before this patch we add Z library to OPENSSL_LIBRARIES when
> BUILD_STATIC is enabled. It is not quite correct: we should do that when
> OPENSSL_USE_STATIC_LIBS is enabled. However we didn't experienced
> problems with static linking of libcurl (when BUILD_STATIC is enabled),
> because OPENSSL_LIBRARIES is added to CURL_LIBRARIES. This is a kind of
> side effect of dependency libcurl on OpenSSL and it is better to add
> libz explicitly to CURL_LIBRARIES (and OPENSSL_LIBRARIES) when
> appropriate.

Is it correct, that all the curl-related changes are just sugar not
related to the bug? Because I spent some time on attempts to understand,
how is curl related to the problem, but looks like in no way.

> 
> Fixes #4437.
> ---
> 
> https://github.com/tarantool/tarantool/issues/4437
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4437-fix-static-openssl-build-full-ci
> 
> I more or less understood what is going on with all that linking stuff,
> but it would be glad if you'll check my wording around this topic.

Looks correct. Just a simple problem of a missing library during static linking
of a subsystem.

[tarantool-patches] Re: [PATCH] build: fix linking with static openssl library

[Alexander Turenko]

> > Before this patch we add Z library to OPENSSL_LIBRARIES when
> > BUILD_STATIC is enabled. It is not quite correct: we should do that when
> > OPENSSL_USE_STATIC_LIBS is enabled. However we didn't experienced
> > problems with static linking of libcurl (when BUILD_STATIC is enabled),
> > because OPENSSL_LIBRARIES is added to CURL_LIBRARIES. This is a kind of
> > side effect of dependency libcurl on OpenSSL and it is better to add
> > libz explicitly to CURL_LIBRARIES (and OPENSSL_LIBRARIES) when
> > appropriate.
> 
> Is it correct, that all the curl-related changes are just sugar not
> related to the bug? Because I spent some time on attempts to understand,
> how is curl related to the problem, but looks like in no way.

It is a kind of refactoring, yep. I had understood the problem better
and so wrote better code to fix it. I would start the paragraph with a
remark like:

> The patch contains changes around linking with libcurl: they does not
> change any behaviour. Consider them as refactoring.

Then I'll describe why it is changed, why it did work before and so:
just like in the current paragraph.

I'll let you know when will update the branch.

WBR, Alexander Turenko.

[tarantool-patches] Re: [PATCH] build: fix linking with static openssl library

[Alexander Turenko]

On Sun, Aug 25, 2019 at 02:15:38PM +0300, Alexander Turenko wrote:
> > > Before this patch we add Z library to OPENSSL_LIBRARIES when
> > > BUILD_STATIC is enabled. It is not quite correct: we should do that when
> > > OPENSSL_USE_STATIC_LIBS is enabled. However we didn't experienced
> > > problems with static linking of libcurl (when BUILD_STATIC is enabled),
> > > because OPENSSL_LIBRARIES is added to CURL_LIBRARIES. This is a kind of
> > > side effect of dependency libcurl on OpenSSL and it is better to add
> > > libz explicitly to CURL_LIBRARIES (and OPENSSL_LIBRARIES) when
> > > appropriate.
> > 
> > Is it correct, that all the curl-related changes are just sugar not
> > related to the bug? Because I spent some time on attempts to understand,
> > how is curl related to the problem, but looks like in no way.
> 
> It is a kind of refactoring, yep. I had understood the problem better
> and so wrote better code to fix it. I would start the paragraph with a
> remark like:
> 
> > The patch contains changes around linking with libcurl: they does not
> > change any behaviour. Consider them as refactoring.
> 
> Then I'll describe why it is changed, why it did work before and so:
> just like in the current paragraph.
> 
> I'll let you know when will update the branch.
> 
> WBR, Alexander Turenko.

I removed the paragraph of the question and added the following one:

 | The patch changes OPENSSL_LIBRARIES variable to fix the issue with
 | static linking of OpenSSL libraries. It also changes CURL_LIBRARIES in
 | the same way, however this does not alter any visible behaviour, because
 | OPENSSL_LIBRARIES is added to CURL_LIBRARIES. The latter change was made
 | to unify the way to choose libraries to link with: it is pure
 | refactoring part.

Hope it clarifies why I touched FindCURL.cmake.

Rebased the branch on top of the current master.

branch: Totktonada/gh-4437-fix-static-openssl-build-full-ci

WBR, Alexander Turenko.

[tarantool-patches] Re: [PATCH] build: fix linking with static openssl library

[Vladislav Shpilevoy]

LGTM.

[tarantool-patches] Re: [PATCH] build: fix linking with static openssl library

[Kirill Yukhin]

Hello,

On 23 авг 05:08, Alexander Turenko wrote:
> System-wide dynamic libraries usually (always?) have NEEDED and RUNPATH
> tags in a dynamic section (as `readelf -d /usr/lib/lib<...>.so` shows),
> so when we link, say, with libssl.so, which depends on libz.so, a linker
> does not complain against unresolved symbols that can be found in Z
> library (if it is installed within a system).
> 
> Things are different when we linking with a static library. Say, when we
> linking with libssl.a, which contains an unresolved symbol from Z
> library, a linker reports an error. It is not possible to store an
> information where to find unresolved symbols (NEEDED / RUNPATH) in a
> static library (AFAIK).
> 
> We depend on three libraries that are depend on Z library: libcurl,
> libssl and libcrypto (two latter are part of OpenSSL). When one of those
> libraries is linked statically we should link with libz.so or libz.a
> (depending on BUILD_STATIC flag). The patch doing exactly this.
> 
> Before this patch we add Z library to OPENSSL_LIBRARIES when
> BUILD_STATIC is enabled. It is not quite correct: we should do that when
> OPENSSL_USE_STATIC_LIBS is enabled. However we didn't experienced
> problems with static linking of libcurl (when BUILD_STATIC is enabled),
> because OPENSSL_LIBRARIES is added to CURL_LIBRARIES. This is a kind of
> side effect of dependency libcurl on OpenSSL and it is better to add
> libz explicitly to CURL_LIBRARIES (and OPENSSL_LIBRARIES) when
> appropriate.
> 
> Fixes #4437.
> ---
> 
> https://github.com/tarantool/tarantool/issues/4437
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4437-fix-static-openssl-build-full-ci

I've checked your patch into 1.10, 2.1, 2.2 and master.

--
Regards, Kirill Yukhin