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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8209
Build 8227: arc lint + arc unit
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
krop added a subscriber: krop.Feb 12 2019, 9:12 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.

In D18943#410391, @cgiboudeaux wrote:

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
57 ↗(On Diff #51473)

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
57 ↗(On Diff #51473)

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
57 ↗(On Diff #51473)

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.

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

QUIET here as well

57 ↗(On Diff #51473)

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.

krop accepted this revision.Mar 2 2019, 11:57 AM

LGTM

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.