Small problem with PolkitQt

[Alexander Neundorf]

On Tuesday 12 January 2010, Alexander Neundorf wrote:
...
> I'll have a closer look at this tomorrow.

Ok, here we go.


> Index: cmake/modules/FindPolkitQt-1.cmake
> ===================================================================
> --- cmake/modules/FindPolkitQt-1.cmake	(revisione 1072643)
> +++ cmake/modules/FindPolkitQt-1.cmake	(copia locale)
> @@ -87,7 +87,7 @@
>  # all listed variables are TRUE
>  find_package_handle_standard_args(PolkitQt-1 DEFAULT_MSG 
>        POLKITQT-1_LIBRARIES POLKITQT-1_INCLUDE_DIR POLKITQT-1_VERSION_OK)
> 
> -mark_as_advanced(POLKITQT-1_INCLUDE_DIR POLKITQT-1_CORE_LIBRARY ... 
> +mark_as_advanced(POLKITQT-1_INCLUDE_DIR POLKITQT-1_CORE_LIBRARY ...


Only those variables which are cache variables can be marked as "advanced", so 
e.g. VERSION_OK and _LIBRARIES don't have to be in this list.
I already changed this in svn.


> @@ -96,4 +96,5 @@
>      endif (NOT PC_POLKITQT-1_PREFIX STREQUAL CMAKE_INSTALL_PREFIX)
>  endif (POLKITQT-1_FOUND)
> 
> -set(POLKITQT-1_POLICY_FILES_INSTALL_DIR
> -     ${CMAKE_INSTALL_PREFIX}/share/polkit-1/actions)
> +set(POLKITQT-1_POLICY_FILES_INSTALL_DIR
> +     ${CMAKE_INSTALL_PREFIX}/share/polkit-1/actions CACHE STRING
> +    "Where policy files generated with polkit-1 will be installed")

It's a good idea to put this into the cache, but please put it in the cache 
without ${CMAKE_INSTALL_PREFIX}. If you do it as it is now, later changes to 
CMAKE_INSTALL_PREFIX in the cache will have no effect on 
POLKITQT-1_POLICY_FILES_INSTALL_DIR anymore, since this will then be already 
in the cache. So put it without that in the cache, just as a relative path 
("share/polkit-1/actions").

I know we put the full path of the install dirs in the cache for KDEs install 
dirs, but first, I'm thinking about changing this, since I think it is not 
necessary (but not before 4.4.0 is out), and second, we only put it in the 
cache if it has been explicitely set to some custom value.

So, please put it in the cache without the prefix. If somebody wants a 
different absolute path, he can still change it to an absolute path in the 
cache.

Same comments for FindPolkitQt.cmake.

> Index: cmake/modules/KDE4Macros.cmake
> ===================================================================
> --- cmake/modules/KDE4Macros.cmake	(revisione 1072643)
> +++ cmake/modules/KDE4Macros.cmake	(copia locale)
> @@ -1271,9 +1271,9 @@
>  # the install phase
>  function(KDE4_INSTALL_AUTH_ACTIONS HELPER_ID ACTIONS_FILE)
> 
> -  if(APPLE)
> +  if(KDE4_AUTH_BACKEND_NAME STREQUAL "APPLE")
>      install(CODE "execute_process(COMMAND
>             ${KDE4_KAUTH_POLICY_GEN_EXECUTABLE} ${ACTIONS_FILE}
>             WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})")
> -  elseif(UNIX)
> +  elseif(KDE4_AUTH_BACKEND_NAME STREQUAL "POLKITQT" 
> +         OR KDE4_AUTH_BACKEND_NAME STREQUAL "POLKITQT-1")
>      set(_output ${CMAKE_CURRENT_BINARY_DIR}/${HELPER_ID}.policy)
>      get_filename_component(_input ${ACTIONS_FILE} ABSOLUTE)
  
Looks good.


>   @@ -1285,13 +1285,7 @@
>                        DEPENDS ${_KDE4_KAUTH_POLICY_GEN_EXECUTABLE_DEP})
>      add_custom_target("actions for ${HELPER_ID}" ALL DEPENDS ${_output})
> 
> -    if (NOT POLKITQT_FOUND)
> -        macro_optional_find_package(PolkitQt)
> -    endif (NOT POLKITQT_FOUND)
> -
> -    if (POLKITQT_FOUND)
> -      install(FILES ${_output} DESTINATION 
> -              ${POLKITQT_POLICY_FILES_INSTALL_DIR})
> -    endif (POLKITQT_FOUND)
> +    install(FILES ${_output} 
> +            DESTINATION ${KDE4_AUTH_POLICY_FILES_INSTALL_DIR})
>   endif()
> Index: CreateKDELibsDependenciesFile.cmake
> ===================================================================
> --- CreateKDELibsDependenciesFile.cmake	(revisione 1072643)
> +++ CreateKDELibsDependenciesFile.cmake	(copia locale)
> @@ -115,3 +115,8 @@
>  # in order to work properly. Used by kwrited.
>  macro_bool_to_01(HAVE_UTEMPTER KDE4_KPTY_BUILT_WITH_UTEMPTER)
> file(APPEND "${CMAKE_CURRENT_BINARY_DIR}/KDELibsDependencies.cmake" 
>     "set(KDE4_KPTY_BUILT_WITH_UTEMPTER  ${KDE4_KPTY_BUILT_WITH_UTEMPTER})")
> +
> +# Append stuff needed by the KAuth framework
> +file(APPEND "${CMAKE_CURRENT_BINARY_DIR}/KDELibsDependencies.cmake" "
> +set(KDE4_AUTH_BACKEND_NAME \"${KDE4_AUTH_BACKEND_NAME}\")
> +set(KDE4_AUTH_POLICY_FILES_INSTALL_DIR 
> +    \"${KDE4_AUTH_POLICY_FILES_INSTALL_DIR}\")")

KDE4_KAUTH_BACKEND_NAME is ok.

How KDE4_AUTH_POLICY_FILES_INSTALL_DIR is not ok.
The information coming from the KDELibsDependencies.cmake file, i.e. written 
by CreateKDELibsDependenciesFile.cmake, gives information about the kdelibs 
which is installed and which has been found. They are no cache variables so 
they cannot be changed by the user later on who builds against kdelibs.

So KDE4_AUTH_POLICY_FILES_INSTALL_DIR is the variable which tells us where 
kdelibs installed its policy files to.
OTOH the project which uses kdelibs is free to install its files to any other 
location. 
The locations recommended for use by KDE (FindKDE4Internal.cmake) are the 
FOO_INSTALL_DIR variables from FindKDE4Internal.cmake.
So there you should add a AUTH_POLICY_FILES_INSTALL_DIR variable. I would 
suggest that this should be handled the same way as the other INSTALL_DIR 
variables.
This would mean if you build foo against kdelibs and install it to the same 
location, AUTH_POLICY_FILES_INSTALL_DIR will point to 
KDE4_AUTH_POLICY_FILES_INSTALL_DIR. 
But if you install it somewhere else, this will be relative to the new 
CMAKE_INSTALL_PREFIX. And if you then set it via -D or write a value you want 
into the cache, you can point it just anyway.
Would this be ok ?
If not, how should it behave ?


> Index: kdecore/auth/ConfigureChecks.cmake
> ===================================================================
> --- kdecore/auth/ConfigureChecks.cmake	(revisione 1072643)
> +++ kdecore/auth/ConfigureChecks.cmake	(copia locale)
> @@ -1,28 +1,34 @@
>  ####### checks for kdecore/kauth ###############

How about setting the non-cache KAUTH_BACKEND variable in all the branches and 
do a 
set(KDE4_KAUTH_BACKEND_NAME ${KAUTH_BACKEND} ... CACHE ... )
just once at the end ?


> Index: kdecore/CMakeLists.txt
> ===================================================================
> --- kdecore/CMakeLists.txt	(revisione 1072643)
> +++ kdecore/CMakeLists.txt	(copia locale)

Looks ok I'd say.

Alex