cmake: don't include non-IMPORTED zlib target in check_library_exists.

[nmoinvaz]

check_library_exists does not work well with non-imported targets. ZLIB might not be an imported target if it is included in a parent cmake project before including curl.

We use a separate variable CURL_DEPS to keep track of non-imported targets so they don't get caught up in the check_library_exists_concat() call.

Original issue: #11285.

Background info: #11285 (comment) and #11537 (comment).

[vszakats]

If I read the changes correctly, CURL_DEPS holds the dependencies that may not be ready yet (OpenSSL and ZLIB). Then we are omitting them from feature checks that do require them, but where that may not have been built yet. This may indeed result in a successful build, but without the features that required those dependencies. This includes LDAP and also HTTP/3, which needs to check for the QUIC feature in OpenSSL. ZLIB is also necessary for several dependencies.

The problem to solve here is a totally reasonable one, but IMO there is no other way around it, than instructing CMake to build leaf dependencies earlier than parents, in the correct order, for the whole tree. For this, CMake should first query all projects it's meant to build for their dependencies, build the dependency tree (report or fail on circular ones) and form the order that avoids this kind of problem in the first place. A "low-cost" version of this may be to somehow trigger building these dependencies from our CMakeLists.txt, to be ready when we need them.

Does CMake offer built-in support for this problem? (or is this meant to be to job of higher level / external tooling?)

[bradking]

@nmoinvaz what @vszakats is saying is that curl cannot be configured correctly unless these dependencies already exist on disk. That means the motivating use case, to vendor curl and its dependencies and build them all in one giant project in a single CMake run, will not work. Only in such a use case would the dependencies be ALIAS targets.

[nmoinvaz]

It may not work in the OpenSSL case (if it does feature checks), but it should work on the zlib case since there are no feature checks.

Even in OpenSSL, I'm not sure why feature checks/flags are not available as #defines. I will investigate.

[nmoinvaz]

It looks like it is using check_symbol_exists to determine if OpenSSL is some variant/fork.

The only use case I really care about is the ZLIB use case. We have been using our own zlib TARGET for a few years on Windows before upgrading to curl 8. I only threw OpenSSL in the mix because I was looking at all the CMake :: libs. But actually the CMake comments for OpenSSL indicates it must be an IMPORTED target. I wonder if there should be a check there to ensure that is the case.

[nmoinvaz]

ZLIB is also necessary for several dependencies.

@vszakats can you explain this a bit. My understanding is that whether or not the CMake targets are imported/non-imported, that all the symbols are resolved in the linker stage.

[vszakats]

ZLIB is a transitive dependency for a list of curl dependencies (OpenSSL, wolfSSL, all SSHs, ngtcp2).

In curl-for-win, ZLIB is the very first project to build for these reasons exactly. This is orchestrated by a custom script and a manually set build order. I'd expect CMake offering control over that if it's capable of building a bunch of projects in a single go.

If we limit this to ZLIB-only, isn't it a solution to "just" make sure in the upper-level CMake logic, that ZLIB is always built before curl?

[nmoinvaz]

If we limit this to ZLIB-only, isn't it a solution to "just" make sure in the upper-level CMake logic, that ZLIB is always built before curl?

My intention is not to change the build order of projects. CMake already handles build order dependency nicely between parent and child projects. Since ZLIB does not have any checks, it does not need to be built beforehand. By adding CURL_DEPS using target_link_libraries CMake knows to build those dependencies first.

The goal is to prevent our zlib CMake target from being included in the call to check_library_exists which throws a CMake error. @bradking had mentioned that it my previous attempted PR #11537 that the code should be changed a different level than in check_library_exists_concat. This PR is that attempt. We have been including curl in our CMake project along with our own zlib project and linking the two using the ZLIB::ZLIB alias. This has been working fine since before curl 8 and according to #11285 we are not the only ones doing it.

[nmoinvaz]

@bagder any chance in getting this merged?

[dg0yt]

Hm, IMO that build script already fakes ZLIB_FOUND and ZLIB_LIBRARY.
Can't it also set preset the cache variable which captures the result of check_library_exists for its fake ZLIB?

[nmoinvaz]

It's not that simple because of the way that check_library_exists is called by appending previously found library names into the current check. So you'd have to set the cache variable for every call to check_library _exists that included zlib and then do that for each platform. It would be too many variables to be practical.

[nmoinvaz]

Is it possible to get this merged into the next release?

[vszakats]

I haven't made extensive tests, but it seems we might have a "cleaner" fix by eliminating the remaining 4 uses of check_library_exists_concat(). None of those 4 checks require ZLIB, and only two may depend on each other (ldap and lber). Only ldap/lber have an obvious dependency, and that dependency (OpenSSL) is supplied to them via CMAKE_REQUIRED_LIBRARIES already.

Does this draft patch solve this Issue? May it create any new ones?:

diff --git a/CMake/Macros.cmake b/CMake/Macros.cmake
index e12bf303f..7ad2f5c40 100644
--- a/CMake/Macros.cmake
+++ b/CMake/Macros.cmake
@@ -23,19 +23,6 @@
 ###########################################################################
 #File defines convenience macros for available feature testing
 
-# This macro checks if the symbol exists in the library and if it
-# does, it prepends library to the list.  It is intended to be called
-# multiple times with a sequence of possibly dependent libraries in
-# order of least-to-most-dependent.  Some libraries depend on others
-# to link correctly.
-macro(check_library_exists_concat LIBRARY SYMBOL VARIABLE)
-  check_library_exists("${LIBRARY};${CURL_LIBS}" ${SYMBOL} "${CMAKE_LIBRARY_PATH}"
-    ${VARIABLE})
-  if(${VARIABLE})
-    set(CURL_LIBS ${LIBRARY} ${CURL_LIBS})
-  endif()
-endmacro()
-
 # Check if header file exists and add it to the list.
 # This macro is intended to be called multiple times with a sequence of
 # possibly dependent header files.  Some headers depend on others to be
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 863d126ac..97b07254b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -380,7 +380,10 @@ if(ENABLE_THREADED_RESOLVER)
 endif()
 
 # Check for all needed libraries
-check_library_exists_concat("socket" connect      HAVE_LIBSOCKET)
+check_library_exists("socket" "connect" "" HAVE_LIBSOCKET)
+if(HAVE_LIBSOCKET)
+  set(CURL_LIBS "socket;${CURL_LIBS}")
+endif()
 
 check_function_exists(gethostname HAVE_GETHOSTNAME)
 
@@ -755,8 +758,12 @@ if(NOT CURL_DISABLE_LDAP)
   if(NOT USE_WIN32_LDAP)
     # Check for LDAP
     set(CMAKE_REQUIRED_LIBRARIES ${OPENSSL_LIBRARIES})
-    check_library_exists_concat(${CMAKE_LDAP_LIB} ldap_init HAVE_LIBLDAP)
-    check_library_exists_concat(${CMAKE_LBER_LIB} ber_init HAVE_LIBLBER)
+    check_library_exists("${CMAKE_LDAP_LIB}" "ldap_init" "" HAVE_LIBLDAP)
+    if(HAVE_LIBLDAP)
+      check_library_exists("${CMAKE_LDAP_LIB};${CMAKE_LBER_LIB}" "ber_init" "" HAVE_LIBLBER)
+    else()
+      check_library_exists("${CMAKE_LBER_LIB}" "ber_init" "" HAVE_LIBLBER)
+    endif()
 
     set(CMAKE_REQUIRED_INCLUDES_BAK ${CMAKE_REQUIRED_INCLUDES})
     set(CMAKE_LDAP_INCLUDE_DIR "" CACHE STRING "Path to LDAP include directory")
@@ -795,8 +802,10 @@ if(NOT CURL_DISABLE_LDAP)
 
       list(APPEND CMAKE_REQUIRED_DEFINITIONS -DLDAP_DEPRECATED=1)
       list(APPEND CMAKE_REQUIRED_LIBRARIES ${CMAKE_LDAP_LIB})
+      set(CURL_LIBS "${CMAKE_LDAP_LIB};${CURL_LIBS}")
       if(HAVE_LIBLBER)
         list(APPEND CMAKE_REQUIRED_LIBRARIES ${CMAKE_LBER_LIB})
+        set(CURL_LIBS "${CMAKE_LBER_LIB};${CURL_LIBS}")
       endif()
 
       check_c_source_compiles("
@@ -843,7 +852,10 @@ endif()
 # Check for idn2
 option(USE_LIBIDN2 "Use libidn2 for IDN support" ON)
 if(USE_LIBIDN2)
-  check_library_exists_concat("idn2" idn2_lookup_ul HAVE_LIBIDN2)
+  check_library_exists("idn2" "idn2_lookup_ul" "" HAVE_LIBIDN2)
+  if(HAVE_LIBIDN2)
+    set(CURL_LIBS "idn2;${CURL_LIBS}")
+  endif()
 else()
   set(HAVE_LIBIDN2 OFF)
 endif()

[nmoinvaz]

@vszakats removing check_library_exists_concat would also solve the problem so long as any check_library_exists check did not include {CURL_LIBS}.

[vszakats]

I don't think we need ${CURL_LIBS} in any check_library_exists calls.

If any of such check happens to require a previously detected lib, I believe we should add the required one explicitly to the specific check it needs it, instead of blindly adding everything detected previously like we do now.