D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


  In D18943#424041 , @kossebau wrote:
  
  > In D18943#424029 , @vkrause 
wrote:
  >
  > > 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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: matthieugras, kossebau, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D18943#424029 , @vkrause wrote:
  
  > What I don't understand though is which FindFontconfig this used to find, 
there is non in plasma-desktop...
  
  
  kdelibs4support provides one.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: matthieugras, kossebau, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


  D19499  fixes that here

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: matthieugras, kossebau, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


  In D18943#423969 , @matthieugras 
wrote:
  
  > 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...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: matthieugras, kossebau, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Matthieu Gras]

matthieugras added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: matthieugras, kossebau, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:e3b6c8708c48: Add Fontconfig find modudle (authored by 
vkrause).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18943?vs=51849=53050#toc

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18943?vs=51849=53050

REVISION DETAIL
  https://phabricator.kde.org/D18943

AFFECTED FILES
  attic/modules/FindFontconfig.cmake
  docs/find-module/FindFontconfig.rst
  find-modules/FindFontconfig.cmake

To: vkrause, cgiboudeaux
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Christophe Giboudeaux]

cgiboudeaux accepted this revision.
cgiboudeaux added a comment.
This revision is now accepted and ready to land.


  LGTM

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  arcpatch-D18943

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause, cgiboudeaux
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


  ping?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause updated this revision to Diff 51849.
vkrause added a comment.


  Search for pkgconfig quietly.

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18943?vs=51524=51849

BRANCH
  arcpatch-D18943

REVISION DETAIL
  https://phabricator.kde.org/D18943

AFFECTED FILES
  attic/modules/FindFontconfig.cmake
  docs/find-module/FindFontconfig.rst
  find-modules/FindFontconfig.cmake

To: vkrause
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> FindFontconfig.cmake:56
> +# in the FIND_PATH() and FIND_LIBRARY() calls
> +find_package(PkgConfig)
> +pkg_check_modules(PC_FONTCONFIG QUIET fontconfig)

QUIET here as well

> apol wrote in FindFontconfig.cmake:57
> 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

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause updated this revision to Diff 51524.
vkrause added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18943?vs=51473=51524

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18943

AFFECTED FILES
  attic/modules/FindFontconfig.cmake
  docs/find-module/FindFontconfig.rst
  find-modules/FindFontconfig.cmake

To: vkrause
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: kossebau, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> vkrause wrote in FindFontconfig.cmake:57
> Does that mean we can rely on pkgconfig? So far I got the impression we have 
> to treat that as optional in ECM code?

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added inline comments.

INLINE COMMENTS

> apol wrote in FindFontconfig.cmake:57
> 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

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18943: Add Fontconfig find modudle

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> FindFontconfig.cmake:57
> +find_package(PkgConfig)
> +pkg_check_modules(PC_FONTCONFIG QUIET fontconfig)
> +

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause updated this revision to Diff 51473.
vkrause added a comment.


  Add since tag.

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18943?vs=51472=51473

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18943

AFFECTED FILES
  find-modules/FindFontconfig.cmake

To: vkrause
Cc: cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Christophe Giboudeaux]

cgiboudeaux added a comment.


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

INLINE COMMENTS

> FindFontconfig.cmake:23
> +# ``Fontconfig::Fontconfig``
> +
> +#=

Missing 'Since'

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18943

To: vkrause
Cc: cgiboudeaux, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18943: Add Fontconfig find modudle

[Volker Krause]

vkrause created this revision.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
vkrause requested review of this revision.

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

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18943

AFFECTED FILES
  find-modules/FindFontconfig.cmake

To: vkrause
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns