Differential D7823

Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM
ClosedPublic
Actions

Authored by marten on Sep 14 2017, 4:45 PM.

Details

Reviewers
krop
Group Reviewers
Frameworks
Build System
Commits
R240:757402282580: Remove FindGLIB2.cmake and FindPulseAudio.cmake from attic
R240:ee5b036c1df4: Add FindGLIB2.cmake and FindPulseAudio.cmake
Summary

These modules are used in a number of places within Frameworks, Plasma and dependencies:

FindGLIB2:

plasma-desktop/applets/kimpanel/cmake/FindGLIB2.cmake
kdelibs4support/cmake/modules/FindGLIB2.cmake
ecm/attic/modules/FindGLIB2.cmake
phonon/cmake/FindGLIB2.cmake
polkit-qt/cmake/modules/FindGLIB2.cmake

FindPulseAudio:

kdelibs4support/cmake/modules/FindPulseAudio.cmake
ecm/attic/modules/FindPulseAudio.cmake
phonon/cmake/FindPulseAudio.cmake

Apart from those in Phonon, each set is all the same apart from minor formatting differences.

These two modules will also be required by KMix when it is finally ported away from KDElibs4Support. Since these common files are used in multiple places, it makes sense to have them included in ECM instead of making local copies.

Test Plan

Built and installed ECM with these two modules added, configured KMix with KDELibs4Support removed.
GLIB2 and PulseAudio are found correctly using the modules installed by ECM.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Lint Skipped
Unit
Unit Tests Skipped
marten created this revision.Sep 14 2017, 4:45 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptSep 14 2017, 4:45 PM
cfeck added a subscriber: cfeck.Sep 14 2017, 5:13 PM

In other words, your suggestion is not to add it to ecm, but to move it from attic/modules to find-modules.

That raises the question, how one is supposed to use the attic modules, without copying them manually.

marten added a comment.Sep 14 2017, 5:17 PM

It's the same source really - the only differences between those in kdelibs4support and ecm/attic are that the former uses endif(same_as_if) and the latter uses endif().
Nothing else within Frameworks, Plasma or applications uses anything from ecm/attic directly.

krop added a subscriber: krop.EditedSep 15 2017, 11:02 AM

-1. They don't match the ECM coding style and code quality (doc, license, endif(), pkgconfig...)

And :

kdelibs4support/cmake/modules/FindGLIB2.cmake
ecm/attic/modules/FindGLIB2.cmake

These two are there for legacy purpose.

phonon/cmake/FindGLIB2.cmake
polkit-qt/cmake/modules/FindGLIB2.cmake

these repos don't use ECM to build.

kdelibs4support/cmake/modules/FindPulseAudio.cmake
ecm/attic/modules/FindPulseAudio.cmake
phonon/cmake/FindPulseAudio.cmake

same thing here.

marten updated this revision to Diff 19570.Sep 15 2017, 4:33 PM

Modules updated with standard header, documentation and copyright; endif() used throughout.

Yes, Phonon does not use ECM but the modules there are very different to those in ECM anyway.
Within Plasma+applications these modules are needed by plasma-desktop (applets/kimpanel and kcms/phonon) as well as KMix. kcms/phonon does not have a local copy, so this will break or a local copy will be needed if it is ported away from KDELibs4Support.

krop added inline comments.Sep 15 2017, 5:28 PM
find-modules/FindGLIB2.cmake
10 ↗(On Diff #19570)

Call it GLIB2_INCLUDE_DIRS.

43 ↗(On Diff #19570)

Not needed

46–49 ↗(On Diff #19570)

Remove this block

52 ↗(On Diff #19570)

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

54 ↗(On Diff #19570)

Rename to GLIB2_INCLUDE_DIRS

59–64 ↗(On Diff #19570)

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

73 ↗(On Diff #19570)

Drop this line

78 ↗(On Diff #19570)

list(APPEND GLIB2_INCLUDE_DIRS "${GLIB2_INTERNAL_INCLUDE_DIR}")

87 ↗(On Diff #19570)

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)

find-modules/FindPulseAudio.cmake
8–19 ↗(On Diff #19570)

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

12 ↗(On Diff #19570)

PulseAudio_INCLUDE_DIRS

14 ↗(On Diff #19570)

PulseAudio_LIBRARIES

50 ↗(On Diff #19570)

not needed

63 ↗(On Diff #19570)

Remove this line

64 ↗(On Diff #19570)

find_package(PkgConfig)

67 ↗(On Diff #19570)

and this one :)

108 ↗(On Diff #19570)

Add an imported target

marten marked 17 inline comments as done.Sep 15 2017, 8:40 PM

Diff updated.

find-modules/FindPulseAudio.cmake
8–19 ↗(On Diff #19570)

PULSEAUDIO -> PulseAudio changed throughout

marten updated this revision to Diff 19576.Sep 15 2017, 8:41 PM
marten marked an inline comment as done.

Updated as per review comments.

marten marked an inline comment as done.Sep 15 2017, 8:42 PM
krop added a comment.Sep 16 2017, 3:25 PM

Looks good. What's your migration plan ?
if you don't want to bump the ECM requirements for applications looking for pulseaudio, you have to set some compatibility/deprecated vars (eg: kmix uses uppercase variables)

find-modules/FindPulseAudio.cmake
109

PulseAudio_FOUND is undefined if you add this block here.

Move it below find_package_handle_standard_args(...)

marten updated this revision to Diff 19611.Sep 17 2017, 2:18 PM

Diff updated to move library target definition after find_package_handle_standard_args.

If these modules are accepted into ECM then KMix needs a further commit anyway, in order to remove the final requirement for KDELibs4Support. So the correct variable names can be set at this point (I have already tested this configuration). Other packages requiring PA and/or GLib may need to do the same when transitioning to use the ECM versions.

marten marked an inline comment as done.Sep 17 2017, 2:18 PM
marten added a comment.Oct 22 2017, 2:15 PM

Ping anyone?

krop accepted this revision.Nov 5 2017, 10:15 AM

No news from the ECM maintainer, please push.
Don't forget to create .rst files in doc/find-module and change 'Since 5.39.0' to 'Since 5.40.0'

This revision is now accepted and ready to land.Nov 5 2017, 10:15 AM
krop added a comment.Nov 5 2017, 10:18 AM
In D7823#164474, @cgiboudeaux wrote:

Don't forget to create .rst files in doc/find-module and change 'Since 5.39.0' to 'Since 5.40.0'

'5.41.0', sorry

Closed by commit R240:ee5b036c1df4: Add FindGLIB2.cmake and FindPulseAudio.cmake (authored by marten). · Explain WhyNov 6 2017, 12:52 PM
This revision was automatically updated to reflect the committed changes.
cfeck added a comment.Nov 6 2017, 12:56 PM

Are the copies in the attic directory still needed for compatibility reasons, or can they get removed?

marten added a comment.Nov 6 2017, 12:59 PM
In D7823#164879, @cfeck wrote:

Are the copies in the attic directory still needed for compatibility reasons, or can they get removed?

No needed by anything within Frameworks, Plasma or Applications as far as I'm aware. Shall I remove them?

krop added a comment.Nov 6 2017, 1:00 PM
In D7823#164880, @marten wrote:
In D7823#164879, @cfeck wrote:

Are the copies in the attic directory still needed for compatibility reasons, or can they get removed?

No needed by anything within Frameworks, Plasma or Applications as far as I'm aware. Shall I remove them?

Yes, please

Revision Contents
Changeset List

Diff 19576

View Options

find-modules/FindGLIB2.cmake

Loading...
View Options

find-modules/FindPulseAudio.cmake

Loading...