KDE/kdelibs/cmake/modules

[Alexander Neundorf]

On Tuesday 26 May 2009, Ralf Habacker wrote:
> SVN commit 972974 by habacker:
>
> added initial support for fixing windows vista uac problem by adding a
> specific manifest file to executables. Vista manifest support is disabled
> by default and could be enabled by setting the KDE4_ENABLE_UAC_MANIFEST
> variable. This support requires kdewin32 >= 0.3.9
>
> The basic idea of this patch was announced at kde-buildsystem mailing list
> http://lists.kde.org/?l=kde-buildsystem&m=124220817129087&w=2 without any
> objections for about two weeks.
>
>
>
>  M  +27 -1     CMakeLists.txt
>  M  +46 -3     KDE4Macros.cmake
>  A             Win32.Manifest.in
>
>
> --- trunk/KDE/kdelibs/cmake/modules/CMakeLists.txt #972973:972974
> @@ -1,5 +1,23 @@
> +if (WIN32)
> +    OPTION(KDE4_ENABLE_UAC_MANIFEST "add manifest to make vista uac happy"
> OFF) +    if (KDE4_ENABLE_UAC_MANIFEST)
> +        if (NOT MT_EXECUTABLE)
> +            find_program(MT_EXECUTABLE mt
> +                PATHS ${KDEWIN32_INCLUDE_DIR}/../bin
> +                NO_DEFAULT_PATH
> +            )
> +        endif (NOT MT_EXECUTABLE)

Please use the "KDE4_" prefix also for MT_EXECUTABLE, so that it is obvious 
that it is set in FindKDE4Internal.cmake:
http://techbase.kde.org/Policies/CMake_Coding_Style#Writing_CMake_Find-modules
"If you need other commands to do special things then it should still begin 
with XXX_."
Why do you have the "if (NOT MT_EXECUTABLE)" around the find_program() call ?
find_program() only searches if MT_EXECUTABLE is not valid, e.g. empty or 
NOTFOUND.



> --- trunk/KDE/kdelibs/cmake/modules/KDE4Macros.cmake #972973:972974
> @@ -30,7 +30,6 @@
>  # Redistribution and use is allowed according to the terms of the BSD
> license. # For details see the accompanying COPYING-CMAKE-SCRIPTS file.
>
> -
>  # This is for versions of automoc4 which don't provide these two macros.
>  # If such a version is used, just use the "old" style automoc handling.
>  if(NOT COMMAND _AUTOMOC4_KDE4_PRE_TARGET_HANDLING)
> @@ -791,6 +790,47 @@
>  endmacro (KDE4_ADD_UNIT_TEST)
>
>
> +# add a  manifest file to executables. This macro is used by
> kde4_add_executable +#
> +# In cmake <= 2.6.4 there is a bug which returns a wrong path from
> +# get_target_property(var <target_name> LOCATION),  when the
> +# OUTPUT_NAME property of a target is set before.
> +#
> +# To workaround  those cases a specific variable should be set before
> +# calling kde4_add_executable as shown by the following example:
> +#
> +# set(xyz_OUTPUT_NAME test)
> +# kde4_add_executable( xyz <source>)
> +# set_target_properties( xyz PROPERTIES OUTPUT_NAME ${xyz_OUTPUT_NAME} )
> +#
> +macro (KDE4_ADD_MANIFEST _target_NAME)

Hmm, this is a new macro.
Please send a patch to kde-buildsystem before adding new macros to these 
files: http://techbase.kde.org/Policies/CMake_Commit_Policy

Is this a "public" macro or just for internal use by kde4_add_executable() ? 

If that's the case, please prefix it with an underscore, so this becomes more 
obvious.

If it is a macro for public use, please add documentation to the top of 
FindKDE4Internal.cmake

> @@ -831,6 +871,10 @@
>        add_executable(${_target_NAME} ${_add_executable_param} ${_SRCS})
>     endif (KDE4_ENABLE_FINAL)
>
> +   IF (KDE4_ENABLE_UAC_MANIFEST)
> +       KDE4_ADD_MANIFEST(${_target_NAME})
> +   ENDIF(KDE4_ENABLE_UAC_MANIFEST)

Please use consistently lower case:
http://techbase.kde.org/Policies/CMake_Coding_Style#Upper.2Flower_casing

Alex