This dependency was too high for Thomas.

Why do you exclude other OSes?

Maybe we should rename namespace for plugins installation from kmymoney to kmymoney-plugins dir instead of workarounding it like that. What do you think?

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

AFAIK, this works only on UNIX. You didn't exclude other OSes.

For ECM >= 5.38 some defaults are already set (https://cgit.kde.org/extra-cmake-modules.git/tree/kde-modules/KDECMakeSettings.cmake#n263)
If this version cannot be set as minimum version it is required to set all related variables

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()

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/

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.

sure.

yes. cmake does not has cross platform support for this yet. On windows there a symbolic link from bin/kmymoney to bin (see https://en.wikipedia.org/wiki/Symbolic_link) e.g.

cd bin
mklink /D kmymoney .

This dependency was too high for Thomas.

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.

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

Don't the entries need to be listed the other way around so that you can have an installed version and one in development, e.g.

$ XDG_DATA_DIRS=$PWD/tmp/usr/local/share:$XDG_DATA_DIRS \
  QT_PLUGIN_PATH=$PWD/lib:$QT_PLUGIN_PATH \

This way, the files in the build dir will be found first.

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.

The ECM_GLOBAL_FIND_VERSION is also used in the kcoreaddons cmake stuff. Is this an issue too?

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.

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.,

task is closed

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 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.