proj-config.cmake generation: only add find_dependency(CURL) for static builds

[rouault]

CC @dg0yt At least one user recently raised the issue about PROJ dragging CURL dependency (can't remember where exactly). I see that in GDAL build, dependencies are only added for static builds, so I guess this is OK for PROJ

[mwtoews]

xref https://github.com/OSGeo/PROJ/pull/3172#issuecomment-1157942986 where issue was raised. Has this been tested locally? (I don't expect it in CI)

[rouault]

Has this been tested locally?

TBH no

(I don't expect it in CI)

AFAICS we have postinstall checks in test/postinstall/c_app/CMakeLists.txt that do "find_package(${USE_PROJ_NAME} REQUIRED CONFIG)". But maybe https://github.com/curl/curl/blob/8dcbb0f19176da937ddb3c85509b8c84086348f1/lib/CMakeLists.txt#L98 bad effect is hidden when the curl dependencies are in system directories, which must be the case in our CI (or perhaps we don't import curl via CMake config files)

[rouault]

Proposed upstream change for curl: https://github.com/curl/curl/pull/9125

[dg0yt]

xref #3172 (comment) where issue was raised. Has this been tested locally? (I don't expect it in CI)

Note:

But now the public dependencies are already out in the wild.

What I meant is: even if this is fixed in CURL now, you will have to deal with versions which don't contain the fix for a few years to come.

[dg0yt]

For PROJ CI, maybe the problem is hidden by the fact that no configuration builds against a CURL package which was built using cmake, with zlib enabled.

[rouault]

@dg0yt I've tested a cmake build (shared) of curl master (so that has the PUBLIC import of its dependencies), build PROJ master (shared) against it and run test/postinstall/test_cmake.sh on the installed PROJ. I don't see any dependency to CURL in PROJ CMake related files apart from the added find_dependency(CURL). So it seems safe for me, at least for shared libs, to not add this find_dependency(). Am I missing something ?

[dg0yt]

ATM I cannot reproduce any of the original issues I had. (Tested linux and mingw now.) So I assumed it is all resolved.

[rouault]

So I assumed it is all resolved.

could we also remove find_dependency(CURL) for static builds?

[dg0yt]

So I assumed it is all resolved.

could we also remove find_dependency(CURL) for static builds?

Now I'm coming back to why I am reluctant to remove it at all: It depends on CURL's linkage, not PROJ's linkage.

A static build of CURL will typically pull OpenSSL::SSL, OpenSSL::Crypo and ZLIB::ZLIB into PROJ's INTERFACE_LINK_LIBRARIES. And there is only one command which gives consumers the righ set of targets matching CURL's configuration: find_dependency(CURL).

[rouault]

I'm quite confused by the topic. I actually figured out that if PROJ is finding CURL through CMake config files, then the build doesn't work properly as CURL_INCLUDE_DIRS and CURL_LIBRARIES are not defined. I need this extra patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 43bfe95c..d797df9b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -218,6 +218,10 @@ if(ENABLE_CURL)
   find_package(CURL REQUIRED)
   if(CURL_FOUND)
     set(CURL_ENABLED TRUE)
+    if(NOT CURL_LIBRARIES AND TARGET CURL::libcurl)
+      set(CURL_INCLUDE_DIRS $<TARGET_PROPERTY:CURL::libcurl,INTERFACE_INCLUDE_DIRECTORIES>)
+      set(CURL_LIBRARIES "CURL::libcurl;$<TARGET_PROPERTY:CURL::libcurl,INTERFACE_LINK_LIBRARIES>")
+    endif()
   else()
     message(SEND_ERROR "curl dependency not found!")
   endif()
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 296f0bb4..a9d38cef 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -160,9 +160,9 @@ add_executable(test_network
   main.cpp
   test_network.cpp)
 if(CURL_ENABLED)
-  include_directories(${CURL_INCLUDE_DIR})
+  include_directories(${CURL_INCLUDE_DIRS})
   target_compile_definitions(test_network PRIVATE -DCURL_ENABLED)
-  target_link_libraries(test_network PRIVATE ${CURL_LIBRARY})
+  target_link_libraries(test_network PRIVATE ${CURL_LIBRARIES})
 endif()
 target_link_libraries(test_network
   PRIVATE GTest::gtest

This is for a shared PROJ to build against a static Curl. I'm not sure if the value of CURL_LIBRARIES having both CURL::libcurl and $<TARGET_PROPERTY:CURL::libcurl,INTERFACE_LINK_LIBRARIES> is expected, but I found it necessary

[dg0yt]

I actually figured out that if PROJ is finding CURL through CMake config files, then the build doesn't work properly as CURL_INCLUDE_DIRS and CURL_LIBRARIES are not defined.

You mean find_package(CURL CONFIG)? Then the result isn't entirely unexpected, because such output variables are usually set from CMake Find modules only (= legacy CMake). Some projects also provide them through config, for compatibility, in particular for CMAKE_FIND_PACKAGE_PREFER_CONFIG. But the modern way is just targets.

Now CURL is special in the way that the latest versions of CMake's FindCURL.cmake will actually load CURL's CMake config, and figure out at everything else from the imported targets.

So if you want to use the variables, you must keep find_package(CURL [MODULE]).

Given that CURL::libcurl already carries include dirs and link libraries to the the targets it is linked to, above code has redundancies.

The real challenge is creating a valid pc file. You can't just write CURL::libcurl. You need to put libcurl into Requires(.private), or resolve the CURL_LIBRARIES link libraries tree (as implemented in GDAL for GDAL_LIBRARIES).

[rouault]

You mean find_package(CURL CONFIG)?

With cmake 3.21.3 or 3.23.0rc1, I don't have to explicitly specify CONFIG and it picks up the CMake config: 'Found CURL: /home/even/install-curl-cmake/lib/cmake/CURL/CURLConfig.cmake (found version "7.85.0-DEV")'

[dg0yt]

With cmake 3.21.3 or 3.23.0rc1, I don't have to explicitly specify CONFIG and it picks up the CMake config: 'Found CURL: /home/even/install-curl-cmake/lib/cmake/CURL/CURLConfig.cmake (found version "7.85.0-DEV")'

That's what I say: "the latest versions of CMake's FindCURL.cmake will actually load CURL's CMake config, and figure out at everything else from the imported targets." i.e. CURL_INCLUDE_DIRS and CURL_LIBRARIES are still valid result variables for this scenario.

[rouault]

i.e. CURL_INCLUDE_DIRS and CURL_LIBRARIES are still valid result variables for this scenario.

well, not for me.... They are not defined. Adding traces to CMake provided FindCURL.cmake, I see I run into

if(NOT CURL_NO_CURL_CMAKE)
  # do a find package call to specifically look for the CMake version
  # of curl
  find_package(CURL QUIET NO_MODULE)
  mark_as_advanced(CURL_DIR)

  # if we found the CURL cmake package then we are done, and
  # can print what we found and return.
  if(CURL_FOUND)
    find_package_handle_standard_args(CURL HANDLE_COMPONENTS CONFIG_MODE)
    return()
  endif()
endif()

and that doesn't define CURL_INCLUDE_DIRS or CURL_LIBRARIES

[dg0yt]

Hm, your are right. The Find module really does not figure at anything else, apart from CURL_VERSION_STRING. Unless the config is skipped with CURL_NO_CURL_CMAKE.

[dotlambda]

The same should now also be done for tiff.

[github-actions[bot]]

The PROJ project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last two months and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add terminal output examples if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 2 months. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the PROJ project can do to help push this PR forward please let us know how we can assist.