DevHeads.net

Review Request: Add cmake config for kdeclarative library.

Review request for kdelibs.

Description
Currently kdeclarative library does not install the KDeclarativeConfig.cmake and KDeclarativeConfigVersion.cmake to ${LIB_INSTALL_DIR}/cmake/KDeclarative. In the current situation any KDE program that want to use it set the "kdeclarative" name hardcoded into CMakeLists.txt, which is not ideal. This patch fixes that problem.

Diffs
experimental/libkdeclarative/CMakeLists.txt 0db647c
experimental/libkdeclarative/KDeclarativeConfig.cmake.in PRE-CREATION
experimental/libkdeclarative/KDeclarativeConfigVersion.cmake.in PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/104140/diff/" title="http://git.reviewboard.kde.org/r/104140/diff/">http://git.reviewboard.kde.org/r/104140/diff/</a>

Testing

Thanks,

Lamarque Vieira Souza

Comments

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 03/02/2012 - 14:01

(Updated March 2, 2012, 6:01 p.m.)

Review request for kdelibs.

Changes
Small changes.

Description (updated)
Currently kdeclarative library does not install the KDeclarativeConfig.cmake and KDeclarativeConfigVersion.cmake to ${LIB_INSTALL_DIR}/cmake/KDeclarative and so other KDE programs cannot find the library using cmake's find_package macro.

kde-workspace, kde-runtime and plasma-mobile currently hardcode the "kdeclarative" library name in their CMakeLists.txt, which is not ideal. Once this patch is submitted we need to fix their CMakeLists.txt to use "find_package(KDeclarative REQUIRED)".

Diffs
experimental/libkdeclarative/CMakeLists.txt 0db647c
experimental/libkdeclarative/KDeclarativeConfig.cmake.in PRE-CREATION
experimental/libkdeclarative/KDeclarativeConfigVersion.cmake.in PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/104140/diff/" title="http://git.reviewboard.kde.org/r/104140/diff/">http://git.reviewboard.kde.org/r/104140/diff/</a>

Testing (updated)
I can now compile kde-workspace/ksmserver without using the custom made FindKDeclarative.cmake.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 03/05/2012 - 17:55

(Updated March 5, 2012, 9:55 p.m.)

Review request for kdelibs.

Changes
Add "set(KDECLARATIVE_FOUND true)" to satisfy macro_log_feature calls.

Description
Currently kdeclarative library does not install the KDeclarativeConfig.cmake and KDeclarativeConfigVersion.cmake to ${LIB_INSTALL_DIR}/cmake/KDeclarative and so other KDE programs cannot find the library using cmake's find_package macro.

kde-workspace, kde-runtime and plasma-mobile currently hardcode the "kdeclarative" library name in their CMakeLists.txt, which is not ideal. Once this patch is submitted we need to fix their CMakeLists.txt to use "find_package(KDeclarative REQUIRED)".

Diffs (updated)
experimental/libkdeclarative/CMakeLists.txt 0db647c
experimental/libkdeclarative/KDeclarativeConfig.cmake.in PRE-CREATION
experimental/libkdeclarative/KDeclarativeConfigVersion.cmake.in PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/104140/diff/" title="http://git.reviewboard.kde.org/r/104140/diff/">http://git.reviewboard.kde.org/r/104140/diff/</a>

Testing
I can now compile kde-workspace/ksmserver without using the custom made FindKDeclarative.cmake.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add cmake config for kdeclarative library.

By Commit Hook at 04/29/2012 - 16:02

This review has been submitted with commit d5b2e642a75ed4f042121326957dff06be80ea91 by Lamarque V. Souza to branch KDE/4.8.

- Commit Hook

On March 5, 2012, 9:55 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Laszlo Papp at 04/27/2012 - 14:31

Ship it!

Ship It!

- Laszlo Papp

On March 5, 2012, 9:55 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Aleix Pol Gonzalez at 04/27/2012 - 07:08

bump. what's the status of this?

- Aleix Pol Gonzalez

On March 5, 2012, 9:55 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Alexander Neundorf at 03/05/2012 - 16:51

experimental/libkdeclarative/KDeclarativeConfig.cmake.in
<http://git.reviewboard.kde.org/r/104140/#comment8985>

You may want to use something like
KDeclarative_SOURCE_DIR, which is defined if there is a project call like
project(KDeclarative)
in the KDeclarative CMakeLists.txt. You could then also check whether this variable is actually set, and error out if not.

experimental/libkdeclarative/KDeclarativeConfig.cmake.in
<http://git.reviewboard.kde.org/r/104140/#comment8986>

I didn't check, but how is INCLUDE_INSTALL_DIR set ?
Is it done by via find_package(KDE4) ?
In this case, INCLUDE_INSTALL_DIR is a relative path under Windows, so the config file wouldn't work properly under Windows.

- Alexander Neundorf

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 04/09/2012 - 19:26

Any more questions about this patch? Can I push it?

- Lamarque Vieira

On March 5, 2012, 9:55 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 03/06/2012 - 06:58

I meant KDeclarative_SOURCE_DIR in my in my last comment, not KDECLARATIVE_INCLUDE_DIRS. Actually the correct name is kdeclarative_SOURCE_DIR according to CMakeCache.txt. I still do not understand why using kdeclarative_SOURCE_DIR is better than the current implementation.

- Lamarque Vieira

On March 5, 2012, 9:55 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Alexander Neundorf at 03/05/2012 - 17:54

KDeclarative_SOURCE_DIR, not KDECLARATIVE_INCLUDE_DIRS.
KDeclarative_SOURCE_DIR is only set if you are inside the expected source tree (which should then contain a respective project() calls).

- Alexander

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 03/05/2012 - 17:51

I do not know, INCLUDE_INSTALL_DIR was already used in libkdeclarative/CMakeLists.txt before I created this patch. I am just using it.

Both KDECLARATIVE_INCLUDE_DIRS and PROJECT_SOURCE_DIR contain the same value. What is the difference in using one or the other?

- Lamarque Vieira

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Laszlo Papp at 03/02/2012 - 14:57

I am a bit of layman in here (thus pardon me), but I would personally prefer a separated location for these config files. Something like either cmake/modules or in the experimental subfolder itself right next to the "CTestConfig.cmake" (I do not personally know what is the best practice here). You can see an example for cmake/modules from Stephen in grantlee, but the cmake example, about it, puts it into the $projectroot. Admittedly, it would be nice to hear some kde-buildsystem guy(s) to comment on this, like Alex, Stephen, etc. :)

Thank you for your effort about it really, by the way!

- Laszlo Papp

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Lamarque V. Souza at 03/05/2012 - 17:51

I still do not get why use a different path than ${LIB_INSTALL_DIR}/cmake/KDeclarative/. Just because the library is experimental? By the way, why is libkdeclarative still experimental? Device Notifier from kde-workspace 4.8.0 already depends on it. Shouldn't we upgrade libkdeclarative to stable?

- Lamarque Vieira

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add cmake config for kdeclarative library.

By Alexander Neundorf at 03/05/2012 - 16:45

Do you mean the location when installed ?
This has to be ${LIB_INSTALL_DIR}/cmake/KDeclarative/ (ok, some small variations on this are also supported).

Alex

- Alexander

On March 2, 2012, 6:01 p.m., Lamarque Vieira Souza wrote: