KDE/kdelibs/cmake/modules

[Dario Freddi]

Gotta go out in a moment, I'll try to be quick

In data sabato 05 settembre 2009 15:47:27, Alexander Neundorf ha scritto:
: > On Saturday 05 September 2009, Dario Freddi wrote:
> > Hi back,
> >
> > first attempt of doing things right. Please review. My only doubt is
> > about the new KDE4_KAUTH_POLICY_GEN_EXECUTABLE: is it valid also while
> > building kdelibs or should I add another if _kdeBootstrap to
> > kde4_auth_add_actions?
> 
> Since you set the variable in both cases in FindKDE4Internal.cmake, it is
>  ok as it is.
> But, in FindKDE4Internal.cmake, just a few lines further where you set the
> variables, there is among others also a _KDE4_MEINPROC_EXECUTABLE_DEP
> variable set. This is then used in custom commands or custom targets as a
> dependency, which enforces that this executable is built before that custom
> command is executed. You need to do the same for
> KDE4_KAUTH_POLICY_GEN_EXECUTABLE.
> 
> 
> 
> About FindPolKitQt.cmake: can you please post the modified file instead of
>  the patch in this case ? Seems like every line has changed :-)

Will do :D

> 
> 
> FindKDE4Internal.cmake/KDE4Macros.cmake/MacroKAuth.cmake:
> 
> Macro KDE4_AUTH_ADD_HELPER():
> A better name for the macro KDE4_AUTH_ADD_HELPER() would be
> KDE4_ADD_AUTH_HELPER_EXECUTABLE().

Makes sense

> Hmm, now actually looking at it, I don't like the macro.
> It does many things at once and that's not obvious from the name.
> 
> E.g. I don't like that it does
> target_link_libraries(... ${KDE4_KDECORE_LIBS})
> No other macro we have adds libraries to an executable, this is always done
> explicitely by the developer.
> (only exceptions are kde4_add_kdeinit_executable() linking the
>  helper-library and kde4_add_executable() linking QtMain under Windows).

You are quite right, better off removing ittt

> 
> Also I don't like that it calls install.
> You know, when I would see the code
> 
> kde4_add_auth_helper_executable(foo ...)
> 
> I would assume that this executable is not installed because there is no
> install rule there.
> 
> How about changing the macro to something like this:
> 
> function(KDE4_INSTALL_AUTH_HELPER_FILES HELPER_ID HELPER_USER)
> 
>   if (_kdeBootStrapping)
>     set(_stubFilesDir  ${CMAKE_SOURCE_DIR}/kdecore/auth/backends/dbus/ )
>   else (_kdeBootStrapping)
>     set(_stubFilesDir  ${KDE4_DATA_INSTALL_DIR}/kauth/ )
>   endif (_kdeBootStrapping)
> 
>   configure_file(${_stubFilesDir}/dbus_policy.stub
>                  ${CMAKE_CURRENT_BINARY_DIR}/${HELPER_ID}.conf)
>   install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${HELPER_ID}.conf
>           DESTINATION ${SYSCONF_INSTALL_DIR}/dbus-1/system.d/)
> 
>   configure_file(${_stubFilesDir}/dbus_service.stub
>                  ${CMAKE_CURRENT_BINARY_DIR}/${HELPER_ID}.service)
>   install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${HELPER_ID}.service
>           DESTINATION ${DBUS_SYSTEM_SERVICES_INSTALL_DIR})
> endfunction(KDE4_INSTALL_AUTH_HELPER)
> 
> As you see, I also replaced macro() by function() (new since cmake 2.6).
>  This has the advantage that variables set inside a function are local to
>  the function. And I think with a function the arguments are real
>  variables, not basically text replacement as with macro (but I'm not sure
>  about this one, I didn't test it, please give it a try).
> 
> Then the code in a CMakeLists.txt would look like:
> 
> kde4_add_executable(foo ....)
> target_link_libraries(foo ...)
> install(TARGETS foo DESTINATION ${LIBEXEC_INSTALL_DIR} )
> 
> kde4_install_auth_helper(...)
> 
> That's more cmake code to to write, but it's more obvious what's going on
> (i.e. less magic, more well-known commands).

Uhm, is there any way we could rename the macro to make it more clear but keep 
the install and add_executable inside? The few developers that used this stuff 
yet provided a great feedback on how easy the helper creation is. But if you 
really think we would better be off this way, I will simply change it (at this 
point the number of programs/stuff using KAuth is still under control)

> 
> 
> Macro KDE4_AUTH_REGISTER_ACTIONS:
> 
> Maybe an "install" in the name of the macro would be a good idea, since it
> installs stuff.

Uhm, you are right. KDE4_AUTH_INSTALL_ACTIONS? But it seems confusing to me. 
REGISTER_AND_INSTALL is making it too long IMHO. Any other suggestions?

> It's not correctly indented.

Will fix

> 
> 
> About the CMakeLists.txt:
> you don't need the RUN_UNINSTALLED keyword, it's ignored since KDE 4.2
>  (I'll commit a patch which corrects the documentation regarding this
>  soon).

Good to know :)

Thanks for the review, I'll wait for you to comment on those 2 points and send 
again an updated diff (and the whole polkitqt file :D)

> 
> 
> Alex
> 

-- 
-------------------

Dario Freddi
KDE Developer
GPG Key Signature: 511A9A3B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20090905/8228140f/attachment.sig