Add Fontconfig find modudle
ClosedPublic

Authored by vkrause on Feb 12 2019, 8:57 AM.

Details

Summary

Originally coming from KWin, but now also needed by qtbase.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vkrause created this revision.Feb 12 2019, 8:57 AM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptFeb 12 2019, 8:57 AM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
vkrause requested review of this revision.Feb 12 2019, 8:57 AM

I'm not sure to understand the commit message, does qtbase look for ECM ?

find-modules/FindFontconfig.cmake
24

Missing 'Since'

vkrause updated this revision to Diff 51473.Feb 12 2019, 9:21 AM

Add since tag.

I'm not sure to understand the commit message, does qtbase look for ECM ?

Not as such at this point, but the CMake port of Qt (see wip/cmake branch in qtbase) for Qt6 currently copies CMake files from multiple KDE repositories, or duplicates existing modules. I'm trying to unify that to just ECM as the canonical source, which should simplify collaboration with the Qt team.

I just realized there is a much older version of this in attic/modules already, should that be removed as part of adding this?

apol added a subscriber: apol.Feb 12 2019, 10:44 AM
apol added inline comments.
find-modules/FindFontconfig.cmake
58

If you use IMPORTED_TARGET you can skip most stuff below. i.e.
pkg_check_modules(Flatpak IMPORTED_TARGET flatpak>=0.11.8)

In fact, I'd argue it's just easier to have pkg_check_modules called upstream rather than having an intermediary in ECM or so.
It's what I did in Discover at least:
https://phabricator.kde.org/source/discover/browse/master/CMakeLists.txt$36

vkrause added inline comments.Feb 12 2019, 10:47 AM
find-modules/FindFontconfig.cmake
58

Does that mean we can rely on pkgconfig? So far I got the impression we have to treat that as optional in ECM code?

apol added inline comments.Feb 12 2019, 10:52 AM
find-modules/FindFontconfig.cmake
58

Maybe you are right, I don't know what problems pkgconfig my incur in that can't be fixed upstream.

Could the indentation perhaps be turned to be 4 spaces while copying it here? While https://community.kde.org/Policies/CMake_Coding_Style#Indentation allows the choice of 2,3, or 4 spaces, using 4 is more in line with the indentation used in C++ sources, so IMHO more expected to read.

Please also add a docs/find-module/FindFontconfig.rst with a line linking this file:

.. ecm-module:: ../../find-modules/FindFontconfig.cmake

so the documentation generation will cover also this new module.

When it comes to pkgconfig no idea myself, but please update https://community.kde.org/Policies/CMake_Coding_Style#.28Not.29_Using_pkg-config if you find those rules do no longer apply.

vkrause updated this revision to Diff 51524.Feb 12 2019, 3:59 PM

Update indentation, add docs link, remove ancient copy from attic.

cgiboudeaux added inline comments.Feb 16 2019, 11:13 AM
find-modules/FindFontconfig.cmake
57

QUIET here as well

58

ECM requires CMake 2.8.12. 'IMPORTED_TARGET' is not available in this version.

vkrause updated this revision to Diff 51849.Feb 16 2019, 11:30 AM

Search for pkgconfig quietly.

This revision is now accepted and ready to land.Mar 2 2019, 11:57 AM
This revision was automatically updated to reflect the committed changes.

This commit breaks plasma-desktop compilation for me (on ubuntu && arch => undefined refrence in plasma-desktop/kcms/kfontinst).

This commit breaks plasma-desktop compilation for me (on ubuntu && arch => undefined refrence in plasma-desktop/kcms/kfontinst).

Confirmed on a clean rebuild here too. So this needs backward compat variables too.

What I don't understand though is which FindFontconfig this used to find, there is non in plasma-desktop...

D19499 fixes that here

What I don't understand though is which FindFontconfig this used to find, there is non in plasma-desktop...

kdelibs4support provides one.

What I don't understand though is which FindFontconfig this used to find, there is non in plasma-desktop...

kdelibs4support provides one.

... which is installed into a non-standard location that is added to the module search path automatically when doing find_package(KDELibs4Support) it seems. Nasty.