Suggestions for code improvement

[Peter-Korobeynikov]

Hi! I happened to use your module in my project, so I will allow myself to make a couple of suggestions.

1. I suggest adding the following function to automate this process and wrapping libfind_process (See https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/How-To-Find-Libraries#using-libfindmacros)

function(find_lib PREFIX)
    # - Try to find 'LIB' with 'PREFIX' named 'NAMES'
    # Once done, this will define:
    #  ${PREFIX}_FOUND - system has 'libname'}
    #  ${PREFIX}_INCLUDE_DIRS - the 'libname' include directories
    #  ${PREFIX}_LIBRARIES - link these to use 'libname'
    set(options)
    set(oneValueArgs)
    set(multiValueArgs DEP DEPS NAMES PATHS INAMES IPATHS)
    cmake_parse_arguments(LIB "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
    set(${PREFIX}_FOUND FALSE)

    # Check & add dependencies
    set(DEPS_INCLUDE_DIRS)
    set(DEPS_LIBRARIES)
    if(LIB_DEP)     # Add single dependency of current package
        list(POP_FRONT LIB_DEP dep)
        # Find another package and make it a dependency of the current package.
        # This also automatically forwards the "REQUIRED" argument.
        # Usage: libfind_package(<prefix> <another package> [extra args to find_package])
        libfind_package(${PREFIX} ${dep} ${LIB_DEP})
        if(${dep}_FOUND)
            list(APPEND DEPS_INCLUDE_DIRS ${dep}_INCLUDE_DIRS)
            list(APPEND DEPS_LIBRARIES ${dep}_LIBRARIES)
        endif()
    endif()
    if(LIB_DEPS)    # Add multiple dependencies of current package
        list(REMOVE_DUPLICATES LIB_DEPS)
        foreach (dep ${LIB_DEPS})
            libfind_package(${PREFIX} ${dep} QUIET)
            if(${dep}_FOUND)
                list(APPEND DEPS_INCLUDE_DIRS ${dep}_INCLUDE_DIRS)
                list(APPEND DEPS_LIBRARIES ${dep}_LIBRARIES)
            endif()
        endforeach()
    endif()
    list(REMOVE_DUPLICATES DEPS_INCLUDE_DIRS)
    list(REMOVE_DUPLICATES DEPS_LIBRARIES)

    # Try to use pkg-config to get hints about paths
    libfind_pkg_check_modules(${PREFIX}_PKGCONF ${LIB_NAMES})

    # Include dir
    if(LIB_INAMES)
        find_path(${PREFIX}_INCLUDE_DIR NAMES ${LIB_INAMES} PATHS ${LIB_IPATHS} ${${PREFIX}_PKGCONF_INCLUDE_DIRS})
        # Set the include dir variables for libfind_process
        # NOTE: Singular variables for this library, plural for libraries this this lib depends on.
        set(${PREFIX}_PROCESS_INCLUDES ${PREFIX}_INCLUDE_DIR ${DEPS_INCLUDE_DIRS})
    else()
        unset(${PREFIX}_INCLUDE_DIR CACHE)  # Nothing to search - unset this
    endif()

    # Library itself
    if(LIB_NAMES)
        find_library(${PREFIX}_LIBRARY NAMES ${LIB_NAMES} 
             PATHS ${LIB_PATHS} ${${PREFIX}_PKGCONF_LIBRARY_DIRS})
        # Set the libraries for libfind_process
        # NOTE: Singular variables for this library, plural for libraries this this lib depends on.
        set(${PREFIX}_PROCESS_LIBS ${PREFIX}_LIBRARY ${DEPS_LIBRARIES})
    else()
        unset(${PREFIX}_LIBRARY CACHE)      # Nothing to search - unset this
    endif()

    # Finally processing
    libfind_process(${PREFIX})

    if(${${PREFIX}_FOUND})
        set (${PREFIX}_INCLUDE_OPTS ${${PREFIX}_INCLUDE_OPTS} PARENT_SCOPE)
        set (${PREFIX}_LIBRARY_OPTS ${${PREFIX}_LIBRARY_OPTS} PARENT_SCOPE)
        set (${PREFIX}_INCLUDE_DIRS ${${PREFIX}_INCLUDE_DIRS} PARENT_SCOPE)
        set (${PREFIX}_LIBRARIES ${${PREFIX}_LIBRARIES} PARENT_SCOPE)
        set (${PREFIX}_FOUND TRUE PARENT_SCOPE)
    endif()

endfunction()

These lines are:

• unset(${PREFIX}_INCLUDE_DIR CACHE)
• unset(${PREFIX}_LIBRARY CACHE) required to disable search when there is no need and allow you to search for the includes and libs paths separately

(*) There may be bugs in the part of the code where dependencies are added, because I didn't test it (I did it for luck)

In this case, the search script itself is extremely simple, for example:

• FindFireBird.cmake

include(${CMAKE_DIR}/ATOM-libfind.cmake)
if(WIN32)
    find_lib(FireBird
            NAMES fbclient_ms
            )
elseif(UNIX)
    find_lib(FireBird
            NAMES fbclient
            )
endif()

• or, for example, like this: FindPostgreSQL.cmake

include(${CMAKE_DIR}/ATOM-libfind.cmake)
if(WIN32)
    find_lib(PostgreSQL
            INAMES libpq-fe.h
            IPATHS ${CMAKE_INCLUDE_PATH} $ENV{Include}
            )
elseif(UNIX)
    find_lib(PostgreSQL
            INAMES libpq-fe.h
            IPATHS /usr/include/*
            )
else()
    MESSAGE(FATAL_ERROR "Unknown System Platform - STOP!!!")
endif()

2. In the libfind_process function in this fragment:

   ...
    # Process all includes and set found false if any are missing
    foreach (i ${includeopts})
        list(APPEND configopts ${i})
        if (NOT "${${i}}" STREQUAL "${i}-NOTFOUND" 
            AND EXISTS "${${i}}"
        )
            list(APPEND includes "${${i}}")
        else()
            set(found FALSE)
            set(missing_headers TRUE)
        endif()
    endforeach()

    # Process all libraries and set found false if any are missing
    foreach (i ${libraryopts})
        list(APPEND configopts ${i})
        if (NOT "${${i}}" STREQUAL "${i}-NOTFOUND" 
            AND EXISTS "${${i}}"
        )
            list(APPEND libs "${${i}}")
        else()
            set (found FALSE)
        endif()
    endforeach()
    ...

i suggest adding a check for the existence of a file or directory. AND EXISTS "${${i}}". otherwise, if the variables are incorrectly set before calling find_package, such as:

    set(LibName_INCLUDE_DIR "")
# or, for example, like this
    set(LibName_INCLUDE_DIR "complete nonsense")

we will "fly" through the search procedure and get the following result with _FOUND=TRUE, but the output will be inoperable (here LibName=MySQL):

-- Found MySQL
== MySQL_FOUND = TRUE
--   MySQL_DEPENDENCIES=
--   MySQL_INCLUDE_OPTS=MySQL_INCLUDE_DIR
--   MySQL_LIBRARY_OPTS=
--   MySQL_INCLUDE_DIRS=complete nonsense
--   MySQL_LIBRARIES=

Or you need to make an "unset" for the resulting variables before processing the search. However, it seems to me that this is not quite correct.

Thank you for your work. Please excuse this awkward intrusion, I'm new to this business. )) With best wishes Peter

[Tronic]

I'm glad to hear that this module is still of use. CMake itself has changed drastically, so the module could definitely use some maintenance overall.

All your suggested changes seem good and I'd definitely welcome a pull request for that. Could you make one?