Bring FindFontconfig.cmake up to ECM standards
ClosedPublic

Authored by vkrause on Dec 12 2018, 6:17 PM.

Details

Summary

This is a preparation step to move this to ECM (or possibly CMake),
which makes sense now that qtbase is using this too (for Qt6).

Diff Detail

Repository
R108 KWin
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.Dec 12 2018, 6:17 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 12 2018, 6:17 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
vkrause requested review of this revision.Dec 12 2018, 6:17 PM
cgiboudeaux added inline comments.
cmake/modules/FindFontconfig.cmake
10

shall be FONTCONFIG_INCLUDE_DIRS

(https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#standard-variable-names)

This link also suggests the variable names shall be exact-case FONTCONFIG_ → Fontconfig_

27–50

Remove this condition. This is not recommended anymore

56–57

Delete this if() / endif(). Not needed anymore since ages.

88

There's no version check, does it matter?

97–104

endif()

118

Remove the dot, feature_summary() adds a comma after the description

vkrause updated this revision to Diff 47613.Dec 15 2018, 11:05 AM

Address review comments.

cgiboudeaux added inline comments.Dec 15 2018, 12:23 PM
cmake/modules/FindFontconfig.cmake
29

You need a fallback method for platforms where pkgconfig isn't usable.

fontconfig.h defines FC_MAJOR, FC_MINOR, FC_REVISION

60

Now that you have Fontconfig_VERSION, please use the second form:

find_package_handle_standard_args(Fontconfig
FOUND_VAR Fontconfig_FOUND
REQUIRED_VARS Fontconfig_LIBRARIES Fontconfig_INCLUDE_DIRS
VERSION_VAR Fontconfig_VERSION)
vkrause updated this revision to Diff 47656.Dec 16 2018, 9:39 AM

Improve version detection.

This revision is now accepted and ready to land.Dec 16 2018, 9:52 AM
This revision was automatically updated to reflect the committed changes.