libcpr fails to build on Windows because zlib isn't found

LunarWatcher commented 1 month ago

Description

When building with CURL_ZLIB=ON, curl fails to build:

-- Enabled curl SSL
-- Using CMake version 3.26.4
-- curl version=[8.7.1]
CMake Error at C:/Strawberry/c/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find ZLIB (missing: ZLIB_LIBRARY) (found version "1.2.11")
Call Stack (most recent call first):
  C:/Strawberry/c/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Strawberry/c/share/cmake-3.26/Modules/FindZLIB.cmake:200 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  build/_deps/curl-src/CMake/Macros.cmake:78 (find_package)
  build/_deps/curl-src/CMakeLists.txt:558 (optional_dependency)

This is in spite of zlib being correctly sourced: by external_zlib.cmake


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        20/07/2024     18:30                curl-build
d-----        20/07/2024     18:30                curl-src
d-----        20/07/2024     18:31                curl-subbuild
d-----        20/07/2024     18:31                zlib-build
d-----        20/07/2024     18:28                zlib-src
d-----        20/07/2024     18:31                zlib-subbuild

I'm not familiar with the inner workings of find_package, but I'd assume this is related to how it uses a CMake module for finding zlib rather than using the target generated by zlib-ng.

Example/How to Reproduce

Clone the cpr repo
Build with cmake, and pass -DCURL_ZLIB=ON

Possible Fix

No response

Where did you get it from?

GitHub (branch e.g. master)

Additional Context/Your Environment

OS: Windows 10 Home, 22H2
Version: 1.10.5

Also verified with GitHub Actions, but not in a clean build of just libcpr

COM8 commented 1 month ago

@LunarWatcher confirmed. Thanks for reporting!

LunarWatcher commented 1 month ago

I ran into a variant of this problem independently with liblzma in a different project. That patch was more complex because libarchive overrides the CMAKE_MODULE_PATH; curl does not, which means overriding FindZLIB.cmake is a lot more easy. Made a quick try at making the same general approach work:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 66a064b..0b9adaf 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -205,7 +205,7 @@ else()
     #     -DCURL_ZLIB=OFF
     # * from CMake script:
     if (CURL_ZLIB OR CURL_ZLIB STREQUAL AUTO OR NOT DEFINED CACHE{CURL_ZLIB})
-        include(cmake/zlib_external.cmake)
+        list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/zlib")
     endif()

     if (CPR_ENABLE_CURL_HTTP_ONLY)
diff --git a/cmake/zlib_external.cmake b/cmake/zlib/FindZLIB.cmake
similarity index 74%
rename from cmake/zlib_external.cmake
rename to cmake/zlib/FindZLIB.cmake
index 1a8eea0..e2f389f 100644
--- a/cmake/zlib_external.cmake
+++ b/cmake/zlib/FindZLIB.cmake
@@ -20,3 +20,13 @@ if(WIN32 AND BUILD_SHARED_LIBS)
     set_target_properties(zlib PROPERTIES DEBUG_POSTFIX "")
     set_target_properties(zlib PROPERTIES SUFFIX ".dll")
 endif()
+
+# Required to force in a CMake >3.4-compatible ZLIB::ZLIB target
+add_library(ZLIB::ZLIB ALIAS zlib)
+
+# Required for compatibility with the built-in FindZLIB
+set(ZLIB_FOUND TRUE)
+set(ZLIB_INCLUDE_DIRS ${zlib_SOURCE_DIR})
+set(ZLIB_LIBRARY zlib)
+set(ZLIB_LIBARARIES ${ZLIB_LIBARY})
+

Note that the subfolder isn't required, it was just a quick hack to avoid forcing an external zlib on dependencies by scoping it outside the default CMAKE_MODULE_PATH. There's probably more stuff that could be done here with respecting system dependencies if enabled, but that is a problem for later.

Unfortunately, it doesn't work because

CMake Error in build/_deps/curl-src/lib/CMakeLists.txt:
  export called with target "libcurl_shared" which requires target "zlib"
  that is not in any export set.

A couple very quick tries with a separate EXPORT target failed (most likely because I have no idea what I'm doing when it comes to install). There's probably a trick to it that I'll try looking at Later:tm:, but me and install have never gotten along, so it'll have to be when I have more time to kill. I'm leaving this here in case someone else wants to poke at it in the meanwhile

This does make it correctly find the targets at least. Compiling is still blocked by install()

libcpr / cpr