Review Request: Remove KDE_NO_PHONON hackery
David Faure
faure at kde.org
Fri Nov 23 11:25:08 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106987/#review22418
-----------------------------------------------------------
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).
interfaces/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106987/#comment17184>
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.
knotify/config/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106987/#comment17183>
This seems to use the wrong variable, and too early (before the find_package(Phonon) that you added below.
knotify/config/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106987/#comment17182>
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.
- David Faure
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/824f34e7/attachment.html>
More information about the Kde-frameworks-devel
mailing list