[kopete-devel] kdenetwork - unnecessary find_package(KdepimLibs REQUIRED)

[Matt Rogers]

On Tuesday 27 October 2009 19:33:54 Maciej Mrozowski wrote:
> On Thursday 22 of October 2009 18:44:15 Alexander Neundorf wrote:
> 
> Sorry for a lag.
> 
> > +if(INSIDE_KDENETWORK)
> >
> > +	find_package(KdepimLibs REQUIRED)
> > +	include_directories(${KDEPIMLIBS_INCLUDE_DIR})
> > +
> > +else(INSIDE_KDENETWORK)
> >
> > Why is kdepimlibs only required when kopete is built as part of
> > kdenetwork
> 
> It's not exactly like this - if you look at
> http://websvn.kde.org/trunk/KDE/kdenetwork/kopete/CMakeLists.txt?revision=1
> 041143&view=markup
> 
> there's already indication, that kopete authors used to (not sure whether
>  it applies anymore) distribute/build kopete as standalone application and
>  check for kdepimlibs in this case.
> 
> > Also, while you are working on this, could you please check which of all
> >  the included CheckSomething.cmake files in kopete/CMakeLists.txt and
> > krdc/CMakeLists.txt are actually used there ?
> 
> Some indeed aren't, and in krdc none are... (see revised patch attached to
> this mail).
> 
> > And I don't really like this one:
> > +if(INSIDE_KDENETWORK)
> >
> > There are at least two other options:
> > +if(NOT ${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
> > which will work not only in kdenetwork, but in all other places too,
> >
> > or, more similar to the first one:
> > +if(kdenetwork_SOURCE_DIR)
> > This exists only when kopete is built as part of kdenetwork, due to the
> > project(kdenetwork)
> > call at the top of kdenetwork/CMakeLists.txt, which defines this variable
> >  (and also kdenetwork_BINARY_DIR)
> 
> I'm not the author of INSIDE_KDENETWORK concept so cannot really comment
>  here, though I agree with you - the less cmake variables created and the
>  more universal approach - the better.
> 
> Revised patch:
> - moved to (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR}) way
>  of detecting standalone build in kopete
> - removed support for standalone krdc compilation - doesn't work well as it
> needs cmake modules from kdenework/cmake anyway.
> - removed unused CheckXXX.cmake includes from kopete
> - moved KdepimLibs check to kopete subdir
> - moved kdenetwork toplevel include_directories (KDEPIMLIBS_INCLUDE_DIR) to
> kopete/CMakeLists.txt and removed duplicated one from
>  kopete/plugins/bonjour - removed duplicated find_package(LibVNCServer) as
>  there's
> macro_optional_find_package(LibVNCServer) already just below
> - minor code style cleanup (FIND_PACKAGE->find_package) in kopete
> 
> Any comments on this? (Urs?)
> (please keep discussion in kde-buildsystem, thanks)
> 

I dislike the STREQUAL stuff. if (NOT INSIDE_KDENETWORK) is about a bazillion 
times easier to read. I'd rather use kdenetwork_SOURCE_DIR or whatever it is 
that gets generated by the "project(kdenetwork)" call. Seriously, does having 
an extra variable actually add that significant of a cost?

Otherwise, I have no problems with this patch. 
-- 
Matt (Not subscribed to kde-buildsystem, please cc me or keep kopete-devel in 
the chain)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20091027/47c8cbc2/attachment-0001.sig