Tronic / cmake-modules

LibFindMacros development repository and other cool CMake stuff
44 stars 17 forks source link

Suggestions for code improvement #4

Open Peter-Korobeynikov opened 3 years ago

Peter-Korobeynikov commented 3 years ago

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 _libfindprocess (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:

(*) 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:


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 commented 3 years ago

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?