KDE/kdevplatform

[Andreas Pakulat]

On 04.08.09 02:01:12, Aleix Pol wrote:
> On Tue, Aug 4, 2009 at 1:23 AM, Andreas Pakulat <apaku at gmx.de> wrote:
> 
> > On 03.08.09 22:57:29, Aleix Pol Gonzalez wrote:
> > > SVN commit 1006580 by apol:
> > >
> > > Properly look for Kompare, the CMake way.
> >
> > Why? IMHO its just unnecessary overhead compared to what "finding
> > kompare" actually means. All we need is find 1 single header file.
> 
> Yes, and in CMake we find using find_package.

find_package is usually used if you have to do a couple of things to
find a given package. Including finding the headers, the libraries and
setting up all the variables.

I don't really see any benefit of having a FindKompare.cmake.

> > > +                        "http://www.caffeinated.me.uk/kompare/" FALSE
> > ""
> > > +                        "Required for difference checking")
> >
> > This website is completely outdated, if we put a webpage here it should
> > at least point to the right sources and list the current developers.
> 
> 
> Ok, I can change to: http://www.google.es/search?q=kompare

How about asking the maintainer what the official address is and if
there's none just put the websvn address from kdesdk/kompare there?

> > > +configure_file( ${CMAKE_CURRENT_SOURCE_DIR}/config-kdevplatform.h.cmake
> > > +                ${CMAKE_CURRENT_BINARY_DIR}/config-kdevplatform.h )
> > > +
> >
> > Even more code to execute.
> 
> I wonder what you mean. Every single cmake project use configure file for
> conditional compilation inside files. I don't think is that much overhead
> anyway. If cmake parsing (either cmake's or our's) is not fast enough is not
> because of this (actually we don't do anything in configure_file).
> I don't think it's superfluous anyway.

Using a configure-file is good if you have lots of HAVE_XXX in various
places/find-modules. But we only have 1 in there, its really, really
easy to just have add_definitions( -DHAVE_KOMPARE ) inside the
language/CMakeLists.txt and the configure_file doesn't add any value to
this - hence its superfluous.
 
> > >  include (KDE4Defaults)
> > >  include (MacroWriteBasicCMakeVersionFile)
> > >  include (MacroLibrary)
> >
> > Why is the above stuff before these? It should be after it.
> 
> I wanted to add it with the other find_packages. If you prefer to have it
> below I'll move it. It is not a problem.

Actually, thinking about it, the find-kompare stuff should be in
language/CMakeLists.txt so its clear which part of kdevplatform actually
uses it.

Andreas

-- 
You will be attacked by a beast who has the body of a wolf, the tail of
a lion, and the face of Donald Duck.