[PATCH] PHONON_INCLUDE_DIR in FindPhonon.cmake ignores $KDEDIR

[Alexander Neundorf]

On Wednesday 14 April 2010, Friedrich W. H. Kossebau wrote:
> Samedi, le 10 avril 2010, à 21:03, vous avez écrit:
> > On Saturday 10 April 2010, Friedrich W. H. Kossebau wrote:
> > > Vendredi, le 9 avril 2010, à 22:52, Andreas Pakulat a écrit:
> > > > On 09.04.10 22:45:41, Friedrich W. H. Kossebau wrote:
> > > > > Vendredi, le 9 avril 2010, à 21:46, vous avez écrit:
> > > > > > On Friday 09 April 2010, Friedrich W. H. Kossebau wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > TechBase [TB] teaches to install Phonon to $KDEDIR (!=$QTDIR).
> > > > > > > So I have done. But I cannot get kdelibs to compile then, it
> > > > > > > only finds the system one, which is an older one.
> > > > > > >
> > > > > > > [TB]
> > > > > > > http://techbase.kde.org/Getting_Started/Build/KDE4/Prerequisite
> > > > > > >s# Ph
> > > > > > >
> > > > > > >on on
> > > > > > >
> > > > > > > FindPhonon.cmake uses
> > > > > > >
> > > > > > > find_path(PHONON_INCLUDE_DIR NAMES phonon/phonon_export.h
> > > > > > >
> > > > > > >   HINTS
> > > > > > >   ${KDE4_INCLUDE_INSTALL_DIR}
> > > > > > >   ${QT_INCLUDE_DIR}
> > > > > > >   ${INCLUDE_INSTALL_DIR}
> > > > > > >   ${QT_LIBRARY_DIR})
> > > > > > >
> > > > > > > But both KDE4_INCLUDE_INSTALL_DIR and INCLUDE_INSTALL_DIR are
> > > > > > > empty at this moment,
> > > > > > >
> > > > > > > message(STATUS
> > > > > > > "${INCLUDE_INSTALL_DIR}###${KDE4_INCLUDE_INSTALL_DIR}")
> > > > > > >
> > > > > > > gives only ###. What is wrong here? Why are both vars empty?
> > > > > >
> > > > > > You could set CMAKE_PREFIX_PATH, this will be prefered over
> > > > > > everything else.
> > > > >
> > > > > Not a prefered solution though, right?
> > > >
> > > > IMHO its the correct solution to tell cmake to search for libs in a
> > > > specific plugin. CMAKE_PREFIX_PATH is the official way with cmake to
> > > > influence how things like find_package, find_path and find_library
> > > > work.
> > >
> > > Okay, and I see TechBase even tells [TB] to extend CMAKE_PREFIX_PATH
> > > with the $KDEDIR, like
> > >
> > > 	export CMAKE_PREFIX_PATH=$KDEDIR:$CMAKE_PREFIX_PATH
> > >
> > > So in the end there is no need for FindPhonon.cmake to use theKDE4_*
> > > stuff, not only because the inbalance between bootstrapping (have to
> > > tell where Phonon sits even if in KDE install dirs) and
> > > non-bootstrapping (don't have to tell).
> > >
> > > Attached patches clean FindPhonon.cmake, also add a message to see
> > > which Phonon lib is actually used,
> >
> > This should be done already by the find_package_handle_standard_args()
> > call, including the version which has been found (only done if something
> > changes, e.g. on the first run, on subsequent runs it's silent).
> > Doesn't it ?
>
> Ah, was distracted by the fact that there is a log for Qt and a lot of
> other stuff on subsequent runs, still.
>
> Is the decision which shows always a log random and historical or is there
> a reason for the different behaviour?
>
> > > and move the find_package(Phonon) back to the
> > > other find_packages, where it belongs.
> >
> > Hmm, I wouldn't commit this.
> > Two things:
> > - having these two additional search directories in FindPhonon.cmake
> > shouldn't hurt, but in many cases help
> > - removing search paths could be considered a source incompatible change,
> > since now a package which found Phonon before may not find it anymore
> > without setting CMAKE_PREFIX_PATH
>
> Hm, increasing a needed version number is also a source incompatible change
> ;)

Well, if the FindKDE4Internal.cmake shipped with version a.b.c o kdelibs says 
that you need version x.y.z of Foo else to build against this version a.b.c 
of kdelibs then this is just necessary.

But assume that you have an application which just uses Foo but not kdelibs. 
So then you need only Foo but not kdelibs, and in this case find_package(Foo) 
should not care that the kdelibs (which is not used now) would require a 
different version of Foo, but still work the same way.

> Alright, I would have gone for cleaned up complexity, but you surely have
> your reasons for a greater focus on backward compatibility, so consider
> this patches rejected :)

Ok.

Alex