Changeset View
Standalone View
CMakeLists.txt
Show All 28 Lines | |||||
29 | 29 | | |||
30 | 30 | | |||
31 | ######################### General Requirements ########################## | 31 | ######################### General Requirements ########################## | ||
32 | 32 | | |||
33 | if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 6.0.0) | 33 | if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 6.0.0) | ||
34 | message(FATAL_ERROR "This version of KMyMoney requires at least gcc 6.0.0 to be built successfully") | 34 | message(FATAL_ERROR "This version of KMyMoney requires at least gcc 6.0.0 to be built successfully") | ||
35 | endif() | 35 | endif() | ||
36 | 36 | | |||
37 | find_package(ECM 5.10 REQUIRED NO_MODULE) | 37 | find_package(ECM 5.10 REQUIRED NO_MODULE) | ||
wojnilowicz: This dependency was too high for Thomas. | |||||
For ECM >= 5.38 some defaults are already set (https://cgit.kde.org/extra-cmake-modules.git/tree/kde-modules/KDECMakeSettings.cmake#n263) if("${ECM_GLOBAL_FIND_VERSION}" VERSION_LESS "5.38.0") set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") if(WIN32) set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") else() set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") endif() elseif(NOT WIN32) set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") endif() habacker: For ECM >= 5.38 some defaults are already set (https://cgit.kde.org/extra-cmake-modules. | |||||
Can you tell me what exactly prevents updating ECM to a newer version ? The latest kmymoney App Image uses KF5 version 5.51, but stays with ECM version 5.10. habacker: > This dependency was too high for Thomas.
Can you tell me what exactly prevents updating ECM… | |||||
The AppImage is build on the KDE CI. My local dev environment is still on openSuSE 42.3 which only comes with ECM 5.32 tbaumgart: The AppImage is build on the KDE CI. My local dev environment is still on openSuSE 42.3 which… | |||||
38 | set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${ECM_MODULE_PATH} ${ECM_KDE_MODULE_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules) | 38 | set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${ECM_MODULE_PATH} ${ECM_KDE_MODULE_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules) | ||
39 | 39 | | |||
40 | include(KDEInstallDirs) | 40 | include(KDEInstallDirs) | ||
41 | include(KDECMakeSettings) | 41 | include(KDECMakeSettings) | ||
42 | # reimplementation because kmymoney app image is bound to ECM 5.36 | ||||
wojnilowicz: Why do you exclude other OSes? | |||||
Because the default settings in ECM are okay for windows (https://cgit.kde.org/extra-cmake-modules.git/tree/kde-modules/KDECMakeSettings.cmake#n263) but not on linux and other unix based os, where shared library normally are expected below lib/ habacker: Because the default settings in ECM are okay for windows (https://cgit.kde.org/extra-cmake… | |||||
43 | if("${ECM_GLOBAL_FIND_VERSION}" VERSION_LESS "5.38.0") | ||||
Maybe we should rename namespace for plugins installation from kmymoney to kmymoney-plugins dir instead of workarounding it like that. What do you think? wojnilowicz: Maybe we should rename namespace for plugins installation from **kmymoney** to **kmymoney… | |||||
No, kmymoney plugins are normally installed into /usr/lib64/qt5/plugins/kmymoney/. Changing it to /usr/lib64/qt5/plugins/kmymoney-plugins/ is obsolate, because the term 'plugins' is already in the path and the subdirectory below plugins should be the application or library name. habacker: No, kmymoney plugins are normally installed into /usr/lib64/qt5/plugins/kmymoney/. Changing it… | |||||
Hi. Big warnings with call for action: ECM_GLOBAL_FIND_VERSION is an internal private variable of ECM that should not be used externally. You want to use instead your own variable here, which you first set and then also pass to your find_package(ECM) call. This is a time-bomb otherwise to blow up in the hands of distributions. kossebau: Hi. Big warnings with call for action: ECM_GLOBAL_FIND_VERSION is an internal private variable… | |||||
The ECM_GLOBAL_FIND_VERSION is also used in the kcoreaddons cmake stuff. Is this an issue too? alex: The ECM_GLOBAL_FIND_VERSION is also used in the kcoreaddons cmake stuff. Is this an issue too? | |||||
Less of an issue, as kcoreaddons is released in sync with ecm, so if ECM_GLOBAL_FIND_VERSION gets removed, kcoreaddons can be fixed in sync as well. kcoreaddons relying on ECM_GLOBAL_FIND_VERSION is broken in other ways though, as the variable would have the value only of the last find_package(ECM x,y,z) call in the chain, which could be anyone else ensuring that ECM is present for their usage of ECM modules. E,g. when doing find_packate(KF5Package), this includes also KF5PackageMacros.cmake, which changes the value for anything following after due to the macro file (in certain condition) calling find_package(ECM 5.83.0 CONFIG REQUIRED) during inclusion. Boom, now everyone sees ECM_GLOBAL_FIND_VERSION == 5.83.0., kossebau: Less of an issue, as kcoreaddons is released in sync with ecm, so if ECM_GLOBAL_FIND_VERSION… | |||||
this phabricator task is closed and done two years ago. If these issues are important, please raise them as a new issue on invent.kde.org. ostroffjh: this phabricator task is closed and done two years ago. If these issues are important, please… | |||||
I am aware that phabricator is no longer the place for new things :) Reason I still make the comment here on the original review request is to highlight the mistake in context, and help anyone else looking at the histpory of the patch to see comments on it in place. kossebau: > task is closed
I am aware that phabricator is no longer the place for new things :) Reason I… | |||||
@kossebau thanks for taking a look and issuing the warning. I just had a quick look and to me it seems that this whole logic is meanwhile unnecessary. Since v5.0.4 KMyMoney requires ECM 5.42 (see commit 6af6317315 and this patch has been added to the repo after v5.0.6 was released. At least that is how I read the git history. tbaumgart: @kossebau thanks for taking a look and issuing the warning. I just had a quick look and to me… | |||||
44 | set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") | ||||
I don't do it that way and I would not like to see these messages each time I build KMyMoney. Please put them in README.cmake wojnilowicz: I don't do it that way and I would not like to see these messages each time I build KMyMoney. | |||||
habacker: sure. | |||||
45 | set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") | ||||
46 | if(WIN32) | ||||
47 | set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") | ||||
48 | else() | ||||
49 | set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") | ||||
50 | endif() | ||||
51 | elseif(NOT WIN32) | ||||
52 | set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib") | ||||
53 | endif() | ||||
54 | | ||||
42 | include(FeatureSummary) | 55 | include(FeatureSummary) | ||
43 | include(CMakeDependentOption) | 56 | include(CMakeDependentOption) | ||
44 | 57 | | |||
45 | include(GenerateExportHeader) | 58 | include(GenerateExportHeader) | ||
46 | include(KMyMoneyMacros) | 59 | include(KMyMoneyMacros) | ||
47 | 60 | | |||
48 | find_package(PkgConfig) | 61 | find_package(PkgConfig) | ||
49 | 62 | | |||
▲ Show 20 Lines • Show All 330 Lines • Show Last 20 Lines |
This dependency was too high for Thomas.