KDE/kdelibs/cmake/modules

[Alexander Neundorf]

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


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().
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).

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


Macro KDE4_AUTH_REGISTER_ACTIONS:

Maybe an "install" in the name of the macro would be a good idea, since it 
installs stuff. 
It's not correctly indented.


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


Alex