Phonon

[Harald Sitter]

Hullos,

On Fri, Mar 11, 2011 at 6:28 PM, Alexander Neundorf <neundorf at kde.org> wrote:
> On Thursday 10 March 2011, Ben Cooksley wrote:
>> I have found recently that I cannot build KDE Runtime. In tracing
>> this, I ended up with an obscure error:
>>
>> CMake Error at
>> /opt/trunk-kde/kde/share/apps/cmake/modules/MacroEnsureVersion.cmake:95
>> (NORMALIZE_VERSION):
>>   NORMALIZE_VERSION Macro invoked with incorrect arguments for macro named:
>>   NORMALIZE_VERSION
>> Call Stack (most recent call first):
>>   phonon/CMakeLists.txt:18 (macro_ensure_version)
>
> When calling normalize_version(), the version variable should be put in
> quotes, this way even if it is empty it at least becomes an empty string
> instead of nothing.
>
> NORMALIZE_VERSION( "${found_version}" found_vers_num )

As far as I can see phonon/CMakeLists.txt in runtime does not call
NORMALIZE_VERSION directly, but macro_ensure_version (with quotes
actually).

> I just had a look at cmake/CMakeLists.txt in phonons git.
>
> Now this does not look good:
>
> set(BUILDSYSTEM_INSTALL_DIR ${SHARE_INSTALL_PREFIX}/phonon-buildsystem/)
> install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/cmake_uninstall.cmake.in
>              ${CMAKE_CURRENT_SOURCE_DIR}/FindAutomoc4.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/FindPackageHandleStandardArgs.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/FindPhonon.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/FindPhononInternal.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/PhononMacros.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/FindQt4.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/MacroEnsureVersion.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/MacroLogFeature.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/MacroOptionalFindPackage.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/MacroPushRequiredVars.cmake
>              ${CMAKE_CURRENT_SOURCE_DIR}/PhononMacros.cmake
>        DESTINATION ${BUILDSYSTEM_INSTALL_DIR})
>
>
> Why do you think that Phonon needs to install that many Find-modules ?

So we do not need to maintain copies of them across 6 repos (Phonon & Backends).

> Second, it makes no sense for package Foo to install a FindFoo.cmake for
> itself.
> So, don't install FindPhonon.cmake.

Ack, fixed.

> Sorry, FindPhonon.cmake itself looks bad, delete it and start from scratch:
>
> if (NOT PHONON_BUILDSYSTEM_DIR)
>    find_program(PC_EXECUTABLE NAMES pkg-config
>    PATH_SUFFIXES bin
>    HINTS
>    ${CMAKE_INSTALL_PREFIX}
>    ONLY_CMAKE_FIND_ROOT_PATH
>    )
>
>
> There is a FindPkgConfig.cmake, why don't you use that ?

Cause it does not allow getting an arbitrary var.

> Beside, Phonon itself uses cmake, so install a cmake Config.cmake file. This
> file can contain all the information about the installed phonon you want.
> There is absolutely no need for *requiring* pkg-config.
> Especially not in a FindPhonon.cmake which is only found if the "searching"
> project already knows where Phonon is.

FindPhonon is copied to every backend (sorta like a placeholder for
the 10 other files), so searching is a non-issue. If someone wants to
finish the cmakeconfig branch though, that would be most welcome :)


>
> ...
>
> if (PHONON_BUILDSYSTEM_DIR)
>    set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PHONON_BUILDSYSTEM_DIR})
>
>    if (Phonon_FIND_REQUIRED)
>        set(_req REQUIRED)
>    endif (Phonon_FIND_REQUIRED)
>    if (PHONON_FIND_QUIETLY)
>        set(_quiet QUIET)
>    endif (PHONON_FIND_QUIETLY)
>
>    find_package(PhononInternal ${_req} ${_quiet})
> else (PHONON_BUILDSYSTEM_DIR)
>    if (_data_DIR)
>        if (Phonon_FIND_REQUIRED)
>            message(FATAL_ERROR "ERROR: FindPhonon.cmake not found in
> ${_data_DIR}")
>        endif (Phonon_FIND_REQUIRED)
>    else (_data_DIR)
>        if (Phonon_FIND_REQUIRED)
>            message(FATAL_ERROR "ERROR: Either pkg-config can not find its
> phonon config, or you are not using a recent enough Phonon version.")
>        endif (Phonon_FIND_REQUIRED)
>    endif (_data_DIR)
> endif (PHONON_BUILDSYSTEM_DIR)
>
> There is the macro find_package_handle_standard_args().
> Use that and remove all the code above.

On my todo.

regards,
Harald