Clean-up requirements: cmake 3.0, qt 5.7, use min versions, deduplicate KF5
ClosedPublic

Authored by kossebau on Mar 17 2018, 6:00 PM.

Details

Summary
  • cmake 2.8.12 is really outdated, 3.0 is minimum in plasma & kf5
  • cmake_minimum_required should be at begin of toplevel CMakeLists.txt
  • bump qt version to 5.7, matching the min Qt version of kf5 5.42
  • with ecm being part of kf5 since early versions, share ${KF5_MIN_VERSION}
  • use QT_MIN_VERSION & KF5_MIN_VERSION also for separate find_package calls
  • deduplicate all KF5 components searched for in unconditionally included subdirs
Test Plan

Still configures and builds with all options OFF & ON

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau requested review of this revision.Mar 17 2018, 6:00 PM
kossebau created this revision.
kossebau edited the summary of this revision. (Show Details)Mar 17 2018, 6:01 PM

This will conflict a little with D10703, but I am willing to help out to adapt things as needed.

nicolasfella accepted this revision.Mar 17 2018, 7:11 PM
nicolasfella added a subscriber: nicolasfella.

Seems reasonable

This revision is now accepted and ready to land.Mar 17 2018, 7:11 PM
This revision was automatically updated to reflect the committed changes.
apol added a subscriber: apol.Mar 19 2018, 12:09 AM

Also the new minimum requirements you added feel a bit random to me. Why is it related to whatever is the minimum required version of plasma or kf5?

plugins/notifications/CMakeLists.txt
1

These dependencies where added here on purpose, to document why is a dependency needed.

In D11418#228932, @apol wrote:

Also the new minimum requirements you added feel a bit random to me. Why is it related to whatever is the minimum required version of plasma or kf5?

Motivation for cmake 3.0: being the same as with other KDE codebases (kf5, plasma) worked one by most people here means a variant less to deal with (consider cmake policies resulting from min versions)
Motivation for Qt 5.7: that is the min requirement of KF 5.42 (old requirement, did not change that), so making this explicit here helps to know if code needs to be if-version-ed.
Motivation for ECM 5.42: ECM & KF5 are released together since early version, it would be surprising if someone had only a lower version of ECM

Does this explicit reasoning seem less random now?

plugins/notifications/CMakeLists.txt
1

This documentation was not documented then :)

Would you want this readded then?
Given the CMakeLists.txt for the plugins are rather short, I found the link lists to be documenting this clearly as well, so I would not have thought there is any need for this. Also never seen this approach in other projects before, so had my delete key easily triggered :)

asturmlechner added inline comments.
plugins/mousepad/CMakeLists.txt
13

KF5Wayland is now required, mistake or is there a reason for this change?

kossebau added inline comments.Mar 22 2018, 4:24 PM
plugins/mousepad/CMakeLists.txt
13

Copy&Paste mistake, thanks for hint, going to fix later today.

kossebau marked an inline comment as done.Mar 22 2018, 4:36 PM
kossebau added inline comments.
plugins/mousepad/CMakeLists.txt
13