[Kde-maemo] kdelibs modularizing (kde-mobile)

[Alexander Neundorf]

On Tuesday 27 April 2010, Kevin Ottens wrote:
> On Monday 12 April 2010 18:51:41 Alexander Neundorf wrote:
> > On Monday 12 April 2010, Kevin Ottens wrote:
> > > On Saturday 10 April 2010 19:34:14 Alexander Neundorf wrote:
> > > > Doesn't look too bad, but I wouldn't commit it before there is
> > > > something which also uses this.
> > >
> > > Well, it's chicken and egg problem. We kind need it in order to start
> > > the efforts of cutting deps and so on which will use the content of
> > > this patch.
> > >
> > > :-)
> > >
> > > Actually, the sooner it gets in, the sooner I can really start doing
> > > the dependency cuts.
> >
> > How about putting it in together with the first use of it ?
>
> Wise man you are. While working on cutting deps in libplasma which is a
> nice guinea pig there, I reworked the cmake related parts.
>
> There's two things the previous patch wasn't up to:
>  - Keeping the high level controlling variable (profile and platform
> features) out of the C++ realm by default;
>  - Easily finding out from cmake what was the profile of kdelibs (which
> other modules forming our platform should follow, kdepimlibs for instance
> but probably others).
>
> So I took a different path, namely shipping an extra
> KDEPlatformProfile.cmake with kdelibs just like we ship a
> KDELibsDependenciesFile.cmake right now.
>
> Please find attached two patches against kdelibs (I left out the purely C++
> related changes in libplasma that Aaron already reviewed).
> kdelibs_cmake_profiles.patch are the changes to our overall kdelibs build
> with the new profile file installation, kdelibs_plasma_profiles.patch shows
> the changes we would do to a lib build to take care of the platform
> features declared by the profile.
>
> Please let me know if they're good to go. On my side they're ready to be
> committed.

kdelibs_cmake_profiles.patch:
Looks good, but I would put the information not in a separate 
KDEPlatformProfile.cmake file, but just in the already existing 
KDELibsDependencies.cmake file.

(if you have a look at that file you'll notice that the filename is just 
a "historic" relict and that it doesn't contain any dependency information 
anymore, this is now in the exported target-files).

...we might actually rename the KDELibsDependencies.cmake to 
KDELibs4Settings.cmake or something like that, and we might also think about 
installing these files somewhere in libs/ instead of share/...
We'll have to make sure there are no compatibility issues if we rename/move 
this stuff.


Just a style note on plasma/CMakeLists.txt:

+if(KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION)
+    set(PLASMA_NO_KDEWEBKIT TRUE)
...
+if(NOT PLASMA_NO_KDEWEBKIT)
+    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)

I would go for positive logic instead of negative, at least for me this is 
easier to understand:

+set(PLASMA_WITH_KDEWEBKIT TRUE)
+if(KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION)
+    set(PLASMA_WITH_KDEWEBKIT FALSE)
...
+if(PLASMA_WITH_KDEWEBKIT)
+    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)


Also, you could set both the include dir and the link library in one place, 
instead of having two if()s:

+if(NOT PLASMA_NO_KDEWEBKIT)
+    include_directories(${CMAKE_SOURCE_DIR}/kdewebkit/)
+    set(PLASMA_OPTIONAL_LIBS ${PLASMA_OPTIONAL_LIBS} kdewebkit)
+endif(NOT PLASMA_NO_KDEWEBKIT)

Alex