Allow cycles in cmake targets

[jjimenezshaw]

Feature description

In conan2 conan-center-index they have to patch GDAL with this code:
https://github.com/conan-io/conan-center-index/blob/master/recipes/gdal/post_3.5.0/patches/3.12.x/2-allow-cycles-in-cmake-targets.patch

in previous versions of GDAL they did a bit more:
https://github.com/conan-io/conan-center-index/blob/master/recipes/gdal/post_3.5.0/patches/3.8.3/2-allow-cycles-in-cmake-targets.patch

(latest changes from conan-io/conan-center-index#28957)

I don't really understand what are those changes doing, but I was asking myself if it would make sense to add them directly in GDAL.

cc @franramirez688 @jcar87

Additional context

No response

[valgur]

I wouldn't recommend adding these to GDAL. They were a band-aid fix for an issue in the CMake config files generated by Conan. This was fixed by an internal refacto of Conan's generators at some point in the last year or so.

Duplicate of #9022.

[jjimenezshaw]

Thanks @valgur
the current patch in conan-io (https://github.com/conan-io/conan-center-index/blob/master/recipes/gdal/post_3.5.0/patches/3.12.x/2-allow-cycles-in-cmake-targets.patch) is much smaller than your PR in #9022 . It is actually one line
I don't know if it is because it has been optimized during these two years, or because that patch is not covering the full problem.

[franramirez688]

Hi @jjimenezshaw,

Thanks for pinging me, and sorry for the delay. Let me explain the patch a little bit more:

diff --git a/cmake/helpers/GdalGenerateConfig.cmake b/cmake/helpers/GdalGenerateConfig.cmake
index debddd7..688b870 100644
--- a/cmake/helpers/GdalGenerateConfig.cmake
+++ b/cmake/helpers/GdalGenerateConfig.cmake
@@ -38,6 +38,7 @@ function(gdal_flatten_link_libraries _result _is_debug_build)
             # new target
             get_target_property(_link_libraries ${_lib} INTERFACE_LINK_LIBRARIES)
             get_target_property(_type ${_lib} TYPE)
+            list(REMOVE_ITEM _link_libraries ${_lib})
             if(_link_libraries AND NOT TYPE STREQUAL "SHARED_LIBRARY")
                 gdal_flatten_link_libraries(_libs "${_is_debug_build}" ${_link_libraries})
             endif()

I guess that this issue is only related to Conan and some legacy CMake target definitions. Using the upstream code without applying the patch, we end up with this:

CMake Error at src/cmake/helpers/GdalGenerateConfig.cmake:96 (if):
  Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  src/cmake/helpers/GdalGenerateConfig.cmake:88 (gdal_evaluate_link_genex)
  src/cmake/helpers/GdalGenerateConfig.cmake:75 (gdal_resolve_link_genex)
  src/cmake/helpers/GdalGenerateConfig.cmake:30 (gdal_process_link_keywords)
  src/cmake/helpers/GdalGenerateConfig.cmake:49 (gdal_flatten_link_libraries)
  src/cmake/helpers/GdalGenerateConfig.cmake:49 (gdal_flatten_link_libraries)

Why? Having a look at this GDAL function:

function(gdal_flatten_link_libraries _result _is_debug_build)
    set(_libs "${${_result}}")
    gdal_process_link_keywords(working_list "${_is_debug_build}" ${ARGN})
    while(working_list)
        list(POP_BACK working_list _lib)
        if(NOT TARGET "${_lib}")
            list(PREPEND _libs "${_lib}")
        elseif(_lib IN_LIST _libs)
            # already resolved, do not repeat
            continue()
        else()
            # new target
            get_target_property(_link_libraries ${_lib} INTERFACE_LINK_LIBRARIES)
            get_target_property(_type ${_lib} TYPE)
            # list(REMOVE_ITEM _link_libraries ${_lib})
            if(_link_libraries AND NOT TYPE STREQUAL "SHARED_LIBRARY")
                gdal_flatten_link_libraries(_libs "${_is_debug_build}" ${_link_libraries})
            endif()
            list(PREPEND _libs "${_lib}")
        endif()
    endwhile()
    set(${_result} "${_libs}" PARENT_SCOPE)
endfunction()

This line:

get_target_property(_link_libraries ${_lib} INTERFACE_LINK_LIBRARIES)

For instance, it's getting for the_lib == PROJ::proj, these INTERFACE_LINK_LIBRARIES == $<$<CONFIG:Release>:>;$<$<CONFIG:Release>:CONAN_LIB::proj_PROJ_proj_proj_RELEASE>;PROJ::proj. Then, it means that the recursive call to gdal_process_link_keywords(working_list "${_is_debug_build}" ${ARGN}) always gets as a working_list the PROJ::proj and the _lib will be PROJ::proj again and again (infinite loop).

Why is the proj library using the same target name as' INTERFACE_LINK_LIBRARIES'? Because the Conan proj recipe defines the same for both the global and the component:

    def package_info(self):
        self.cpp_info.set_property("cmake_file_name",  "proj")
        self.cpp_info.set_property("cmake_target_name", "PROJ::proj")
        # ....
        self.cpp_info.components["projlib"].set_property("cmake_target_name", "PROJ::proj")

That results in:

    ########## AGGREGATED GLOBAL TARGET WITH THE COMPONENTS #####################
    set_property(TARGET PROJ::proj APPEND PROPERTY INTERFACE_LINK_LIBRARIES PROJ::proj)

Given all those facts, adding the line list(REMOVE_ITEM _link_libraries ${_lib}) solves that boilerplate CMake target.
We'll indeed have to check if we can remove those targets without breaking any users, because it looks like the legacy behavior, but maybe that patch line provides more robustness to the whole gdal_flatten_link_libraries function, and it's harmless enough?