Review Request: Remove KDE_NO_PHONON hackery

[Jon Severinsson]

> On Nov. 23, 2012, 11:25 a.m., David Faure wrote:
> > Great.
> > 
> > I'm surprised that you say that these worked before:
> >    Qt5, Phonon 4 on Qt4*
> >    Qt5, Phonon 5 on Qt4*
> > With the code in the toplevel CMakeLists.txt, there was no way to use any phonon in KF5+Qt5. So I'm glad this is going in, so that all the code can be compiled with Qt5 (until now my qt5-build didn't build everything that my qt4-build was building).
> >

With those setup, kdeframeworks would compile, but of course not link to libphonon.so (as all modules doing such linking would not be included due to the KDE_NO_PHONON hack).

Without phonon, kdeframeworks would not compile at all (due to a find_package(Phonon REQUIRED) in cmake/modules/FindKDE4Internal.cmake and an #include in knotify/config/knotifyconfigactionswidget.cpp). That was what got me started on Phonon in the first place, I didn't want to mix Qt4 and Qt5 headers on the same system...


> On Nov. 23, 2012, 11:25 a.m., David Faure wrote:
> > interfaces/CMakeLists.txt, line 6
> > <http://git.reviewboard.kde.org/r/106987/diff/2/?file=95977#file95977line6>
> >
> >     Shouldn't the find_package(Phonon) be done before this line, rather than inside interfaces/kmediaplayer?
> >     
> >     I'm not sure yet how we'll split up this directory, to be honest. But since the toplevel kdelibs goes away for sure, a find_package is definitely needed here.

I assume interfaces/CMakeLists.txt will disappear together with the top-level CMakeLists.txt.  I only added find_package(Phonon REQUIRED) to interfaces/kmediaplayer/CMakeLists.txt in order to make sure whoever eventually does the splitting remember it.

Please note that both interfaces/CMakeLists.txt and interfaces/kmediaplayer/CMakeLists.txt currently only work when called from the toplevel CMakeLists.txt, so this patch changes nothing in that regard.


> On Nov. 23, 2012, 11:25 a.m., David Faure wrote:
> > knotify/config/CMakeLists.txt, line 2
> > <http://git.reviewboard.kde.org/r/106987/diff/2/?file=95980#file95980line2>
> >
> >     This seems to use the wrong variable, and too early (before the find_package(Phonon) that you added below.

Yes, I didn't clean up existing stuff, as I thought such cleanup should be done when splitting.  I only added find_package(Phonon) to knotify/config/CMakeLists.txt in order to make sure whoever eventually does the splitting remember it.

That said, doing just that cleanup is not too difficult, so I'll do it now instead.


> On Nov. 23, 2012, 11:25 a.m., David Faure wrote:
> > knotify/config/CMakeLists.txt, line 6
> > <http://git.reviewboard.kde.org/r/106987/diff/2/?file=95980#file95980line6>
> >
> >     This will lead to compiler warnings/errors when phonon isn't found, because #if HAVE_PHONON will say "not defined!".
> >     
> >     The proper way to do this is a configure_file for a knotify-config.h, with #cmakedefine01 in the knotify-config.h.cmake, like in many other places in kdelibs.
> >     
> >     Or maybe Phonon is just required for this code.

It compiled, so obviously only a warning. I thought a configure_file was overkill for a single variable, but I'll add one anyway.


- Jon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106987/#review22418
-----------------------------------------------------------


On Nov. 23, 2012, 10:49 a.m., Jon Severinsson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106987/
> -----------------------------------------------------------
> 
> (Updated Nov. 23, 2012, 10:49 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
>     Previously the Phonon headers and library was always required to build,
>     but when building with Qt5 the build system made sure never to link
>     against the library. Thus the build would work (with some missing
>     features) even if the only available phonon was built with Qt4. (Which
>     was the only possibility, as Phonon previously didn't build with Qt5).
>     
>     With this patch, the Phonon headers and library is optional to build,
>     but if they are found they will be used. Thus building with Qt5 and a
>     Phonon built with Qt4 will no longer work. However, this patch allows
>     building with Phonon on Qt5, provided Phonon was built with Qt5.
>     
>     In other words, these combinations are now supported (* are new with this patch):
>     Qt4, No Phonon*
>     Qt4, Phonon 4 on Qt4
>     Qt4, Phonon 5 on Qt4
>     Qt5, No Phonon*
>     Qt5, Phonon 5 on Qt5*
>     
>     While these combinations are not supported (* previously (sort of) worked):
>     Qt4, Phonon 4 on Qt5
>     Qt4, Phonon 5 on Qt5
>     Qt5, Phonon 4 on Qt4*
>     Qt5, Phonon 5 on Qt4*
>     Qt5, Phonon 4 on Qt5
>     
>     Please note that Phonon 5 requires the patch from review 106970 to
>     build on Qt5, and that "Qt5, Phonon 4 on Qt5" is fine as far as kdelibs
>     is concerned, but Phonon 4 won't actually build on Qt5.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt af6c3ec 
>   cmake/modules/FindKDE4Internal.cmake f99aaee 
>   interfaces/CMakeLists.txt d496d01 
>   interfaces/kmediaplayer/CMakeLists.txt 4413131 
>   khtml/CMakeLists.txt db08b27 
>   knotify/config/CMakeLists.txt ae0933d 
>   knotify/config/knotifyconfigactionswidget.cpp de892fe 
> 
> Diff: http://git.reviewboard.kde.org/r/106987/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jon Severinsson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20121123/09a7a398/attachment.html>