D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> FindGLIB2.cmake:10
> +#     True if the GLib2 library is available
> +# ``GLIB2_INCLUDE_DIR``
> +#     The GLib2 include directory

Call it GLIB2_INCLUDE_DIRS.

> FindGLIB2.cmake:43
> +#
> +# For details see the accompanying COPYING-CMAKE-SCRIPTS file.
> +#=============================================================================

Not needed

> FindGLIB2.cmake:46-49
> +if(GLIB2_INCLUDE_DIR AND GLIB2_LIBRARIES)
> +    # Already in cache, be silent
> +    set(GLIB2_FIND_QUIETLY TRUE)
> +endif()

Remove this block

> FindGLIB2.cmake:52
> +find_package(PkgConfig)
> +pkg_check_modules(PC_LibGLIB2 QUIET glib-2.0)
> +

nitpick : the pkgconfig file is called glib-2.0.pc and the module FindGLIB2.cmake.
What about using PC_GLIB2 instead ?

> FindGLIB2.cmake:54
> +
> +find_path(GLIB2_MAIN_INCLUDE_DIR
> +          NAMES glib.h

Rename to GLIB2_INCLUDE_DIRS

> FindGLIB2.cmake:59-64
> +find_library(GLIB2_LIBRARY
> +             NAMES glib-2.0
> +             HINTS ${PC_LibGLIB2_LIBDIR}
> +)
> +
> +set(GLIB2_LIBRARIES ${GLIB2_LIBRARY})

Do the opposite : use GLIB2_LIBRARIES everywhere and add a comment about GLIB2_LIBRARY being deprecated.

> FindGLIB2.cmake:73
> +
> +set(GLIB2_INCLUDE_DIR "${GLIB2_MAIN_INCLUDE_DIR}")
> +

Drop this line

> FindGLIB2.cmake:78
> +if(GLIB2_INTERNAL_INCLUDE_DIR)
> +  set(GLIB2_INCLUDE_DIR ${GLIB2_INCLUDE_DIR} "${GLIB2_INTERNAL_INCLUDE_DIR}")
> +endif()

list(APPEND GLIB2_INCLUDE_DIRS "${GLIB2_INTERNAL_INCLUDE_DIR}")

> FindGLIB2.cmake:87
> +  URL "https://wiki.gnome.org/Projects/GLib"
> +  DESCRIPTION "Event loop and utility library")

set and add a comment about GLIB2_INCLUDE_DIR being deprecated and add a target eg:

  if(GLIB2_FOUND AND NOT TARGET GLIB2::GLIB2)
    add_library(GLIB2::GLIB2 UNKNOWN IMPORTED)
    set_target_properties(GLIB2::GLIB2 PROPERTIES
      IMPORTED_LOCATION "${GLIB2_LIBRARIES}"
      INTERFACE_INCLUDE_DIRECTORIES "${GLIB2_INCLUDE_DIRS}"
    )      
  endif()

(+ doc about it)

> FindPulseAudio.cmake:8-19
> +# ``PULSEAUDIO_FOUND``
> +#      True if the system has the PulseAudio library of at least
> +#      the minimum version specified by either the version parameter
> +#      to find_package() or the variable PULSEAUDIO_MINIMUM_VERSION
> +# ``PULSEAUDIO_INCLUDE_DIR``
> +#      The PulseAudio include directory
> +# ``PULSEAUDIO_LIBRARY``

It's a new module, change PULSEAUDIO to PulseAudio to match the module name.

> FindPulseAudio.cmake:12
> +#      to find_package() or the variable PULSEAUDIO_MINIMUM_VERSION
> +# ``PULSEAUDIO_INCLUDE_DIR``
> +#      The PulseAudio include directory

PulseAudio_INCLUDE_DIRS

> FindPulseAudio.cmake:14
> +#      The PulseAudio include directory
> +# ``PULSEAUDIO_LIBRARY``
> +#      The PulseAudio libraries for linking

PulseAudio_LIBRARIES

> FindPulseAudio.cmake:50
> +#
> +# For details see the accompanying COPYING-CMAKE-SCRIPTS file.
> +#=============================================================================

not needed

> FindPulseAudio.cmake:63
> +
> +if (NOT WIN32)
> +   include(FindPkgConfig)

Remove this line

> FindPulseAudio.cmake:64
> +if (NOT WIN32)
> +   include(FindPkgConfig)
> +   pkg_check_modules(PC_PULSEAUDIO QUIET libpulse>=${PulseAudio_FIND_VERSION})

find_package(PkgConfig)

> FindPulseAudio.cmake:67
> +   pkg_check_modules(PC_PULSEAUDIO_MAINLOOP QUIET libpulse-mainloop-glib)
> +endif()
> +

and this one :)

> FindPulseAudio.cmake:108
> +  URL "https://www.freedesktop.org/wiki/Software/PulseAudio"
> +  DESCRIPTION "Sound server, for sound stream routing and mixing")

Add an imported target

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D7823

To: marten, #frameworks, #build_system
Cc: cgiboudeaux, cfeck, heikobecker
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20170915/2b4dfb40/attachment-0001.html>