Review Request: Don't lose the original CMAKE_MODULE_PATH

[Alexander Neundorf]

On Wednesday 08 August 2012, Patrick Spendrin wrote:
> Am 08.08.2012 22:52, schrieb Alexander Neundorf:
> > On Tuesday 07 August 2012, Patrick Spendrin wrote:
> >> Am 07.08.2012 22:42, schrieb Alexander Neundorf:
> > ...
> > 
> >> > Setting CMAKE_REQUIRED_INCLUDES and CMAKE_REQUIRED_LIBRARIES within
> >> > the
> >> > 
> >> > Config.cmake file is not good style.
> >> > 
> >> > 
> >> > 
> >> > Finding a package should not change the behaviour of cmake, just
> >> > provide
> >> > 
> >> > information about the package which has been found. So I'd prefer to
> >> > 
> >> > remove lines 26 and 27.
> >> 
> >> Since kdewin is a special library (a compatibility layer) I would like
> >> 
> >> to keep that in place. Otherwise I would have to set this in each and
> >> 
> >> every dependent package. The code has been copied over from the old
> >> 
> >> FindKDEWin.cmake, and I think that the behaviour should not be changed
> >> 
> >> here.
> > 
> > Strictly speaking, from the CMake POV, this is wrong.
> > 
> > Doing
> > 
> > find_package(KDEWin)
> > 
> > should not change any current build settings, but only provide
> > information about the installed project.
> > 
> > A user has to do anyway
> > 
> > find_package(KDEWin)
> > 
> > so it shouldn't be a big problem to do the following:
> > 
> > find_package(KDEWin)
> > 
> > set(CMAKE_REQUIRED_INCLUDES ${KDEWIN_INCLUDES} )
> > 
> > set(CMAKE_REQUIRED_LIBRARIES ${KDEWIN_LIBRARIES} )
> > 
> > This would be the clean way.
> > 
> > If there is a chance to break compatibility, this is now with the switch
> > to KF5/e-c-m.
> > 
> > I'm strongly in favor of this.
> > 
> > I know that
> > 
> > find_package(KDE4)
> > 
> > does not follow this rule.
> > 
> > This was not a good decision, but we had to stay with it to keep source
> > compatibility. And it is more convenient for our KDE developers, but it
> > is not how cmake stuff should behave.
> > 
> > With KF5 this changes somewhat. If you do
> > 
> > find_package(KF5)
> > 
> > no settings will be modified, you have to request that explicitely:
> > 
> > find_package(KF5 COMPONENT CMake Compiler)
> > 
> > By requesting the components "CMake" and "Compiler" the cmake- and
> > compiler-settings suggested by KDE will be applied. So it is at least a
> > somewhat explicit request by the user.
> 
> Well, kdewin has no other use case than providing additional include
> headers in constrast to KDE4 and KF5.

More or less every library has as only purpose to provide additional include 
headers and libs ;-)

Somewhere it is probably stated more clearly and to the point, but the 
find_package() documentation says at least:
"find_package()...
Finds and loads settings from an external project. <package>_FOUND will be set 
to indicate whether the package was found. When the package is found package-
specific information is provided through variables documented by the package 
itself."

An installed project should not make assumptions about the projects which will 
be using it.
Maybe the developer wants all find_package() calls in the top level 
CMakeLists.txt, but wants to use kdewin only in some subdir.
By only providing variables this is possible, the developer is completely free 
to use them as he wants to.
And there will be no surprises, since it will only do what he explicitely 
writes.
This is again convenience/lazyness ;-) vs. more code to type/being explicit.

You can still provide a FindKDEWin.cmake, which mostly does
find_package(KDEWin NO_MODULE)
and then maybe sets those CMAKE_REQUIRED_* variables, which I still wouldn't 
like a lot, but at least then it would be possible for a developer to skip 
that automatic setting by doing
find_package(KDEWin NO_MODULE)
directly in his project.

> I will change the Config.cmake file again tomorrow, although I really
> hate that change as it will increase the amount of code each project has
> to include, thus increasing error likeliness etc.

OTOH, it will make it more obvious what is going on, which usually helps with 
understanding and debugging etc.

Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20120809/7654e5a7/attachment.html>