In cmake macro file use CMAKE_CURRENT_LIST_DIR consequently instead of mixed use with KF5I18n_DIR
ClosedPublic

Authored by habacker on Jun 7 2018, 1:15 PM.

Details

Summary

On runtime KF5I18n_DIR points to the same path as CMAKE_CURRENT_LIST_DIR,
but adds an additional hidden and obsolate level of indirection.

Test Plan

compiled and tested with client package

Diff Detail

Repository
R249 KI18n
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
habacker created this revision.Jun 7 2018, 1:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 7 2018, 1:15 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
habacker requested review of this revision.Jun 7 2018, 1:15 PM
krop added a subscriber: krop.Jun 7 2018, 2:42 PM

-1, this change would break the autotests.

In D13406#275375, @cgiboudeaux wrote:

-1, this change would break the autotests.

Hmmh, https://cmake.org/cmake/help/v3.0/variable/CMAKE_CURRENT_LIST_DIR.html mentions "... that As CMake processes the listfiles in your project this variable will always be set to the directory where the listfile which is currently being processed (CMAKE_CURRENT_LIST_FILE) is located." so it should work. Adding message("++++ ${CMAKE_CURRENT_LIST_DIR"} to KF5I18nMacros.cmake returns ++++++ <build-root>/autotests/cmake, which looks correct.
The failure happens on running make test, which inside runs make install in the build dir of the configured ki18_install autotests (located in <buildroot>/autotests/ki18n_install), where -P ${CMAKE_CURRENT_LIST_DIR}/build-tsfiles.cmake is expanded to -P <ki18n-src-root>/cmake/build-pofiles.cmake - Seems to be a cmake bug

habacker updated this revision to Diff 35821.Jun 8 2018, 12:04 PM

Fixed autotests

krop added a comment.EditedJun 8 2018, 12:09 PM

Fixed autotests

Still -1, the code is now more complicated just for a cosmetic change in a build system file. Are you trying to fix anything ?

krop added a comment.Jun 8 2018, 12:10 PM
In D13406#275765, @cgiboudeaux wrote:

Fixed autotests

Still -1, the code is now more complicated just for a cosmetic change in a build system file. Are you trying to fix anything ?

s/not/now

The macro file uses something like`set(_ki18n_uic_script ${CMAKE_CURRENT_LIST_DIR}/kf5i18nuic.cmake) ` for two variables, saying that the referenced files are located beside the macro file, but the remaining files are based surprisely on a complete different variable, hiding where the related files are located really. This patches cleans up this by moving all related files into the build dir, which looks more clean to me. For installing local translations it is only required to add <build-dir>/cmake to use the macro file and for autotests/ki18n_install.

Furthermore KF5I18n_DIR seems to be required to support autotests/ki18n_install, which is a client package using a non standard way to use the macro file from an partially installed ki18n package (some files from <build-root>/cmake and some from <src-root>/cmake) , where it would be much more standard to use a full 'find_package' call inside (https://cgit.kde.org/ki18n.git/tree/autotests/ki18n_install/CMakeLists.txt) only from <build-root>/cmake e.g.

project(ki18n_install)
cmake_minimum_required(VERSION 3.0)

find_package(KF5I18N)

ki18n_install(${CMAKE_CURRENT_SOURCE_DIR}/po)

BTW: I was faced to this on trying to understand the ki1i8n build system, from which I took the macro file and all related dependencies to install po files from a tarball released by KDE for a cross build umbrello for Windows (see https://build.opensuse.org/package/show/windows%3Amingw%3Awin32/mingw32-umbrello), which cant use the full KI18n package for now.

krop accepted this revision.EditedJun 26 2018, 9:55 AM

BTW: I was faced to this on trying to understand the ki1i8n build system, from which I took the macro file and all related dependencies to install po files from a tarball released by KDE for a cross build umbrello for Windows (see https://build.opensuse.org/package/show/windows%3Amingw%3Awin32/mingw32-umbrello), which cant use the full KI18n package for now.

So, you're using files from the ki18n build system, installed in cmake/KF5I18n/ to indicate they're not supposed to be used for another purpose to build a KDE4 app... well ok.

CMakeLists.txt
59–60

file(COPY ...

is fine for the 4 files. No need to replace it

This revision is now accepted and ready to land.Jun 26 2018, 9:55 AM
This revision was automatically updated to reflect the committed changes.
habacker marked an inline comment as done.

issued fixed and submitted rebased patch, thanks for reviewing