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

[Alexander Neundorf]

On Wednesday 28 October 2009, Matt Rogers wrote:
> 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

See at the enmd of this mail.

> > - removed support for standalone krdc compilation - doesn't work well as
> > it needs cmake modules from kdenework/cmake anyway.

The krdc developers should comment on this.

> > - 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

Ok from my side.

> > 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?

No, but it would be nicer to use one of the two more generic methods, where 
you don't create a new mechanism.

Alex