Bug 175427 – [CMake] Fix broken use of REQUIRED with find modules

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

From bug #175426:

FindWebP failed to find libwebp on my test system and printed an error, but I guess it must have set WEBP_FOUND=1 or something because the build proceeded even though it was REQUIRED.

Konstantin discovered that REQUIRED was not working properly:

> No, by design it should result in fatal error. However, I've found why it
> doesn't happen for WebP and GeoClue: package names are case-sensitive, so
> WebP != WEBP, and WEBP_FOUND being false is ignored because cmake looks for
> WebP_FOUND. We either need to rename FindWebP.cmake to FindWEBP.cmake and
> use find_package(WEBP), or use "WebP" spelling consistently. Same for other
> packages with mixed case names.

So we need to call in the case-sensitivity police to fix OptionsGTK.cmake (and probably other ports, too).

[Konstantin Tokarev]

What way of fixing do you prefer? I think it would be more cmake'ish[*] to use mised case like "WebP" everywhere, but renaming files would result in less changes.

[*] more consistent with built-in cmake modules

[Michael Catanzaro]

I have no strong preference. Do want to write the patch, or shall I?

Are we going to have to change WEBP_LIBRARIES and WEBP_INCLUDE_DIRS to WebP_LIBRARIES and WebP_FOUND as well?

[Konstantin Tokarev]

Huh, there are a quite a few of them:

FindGTKUnixPrint.cmake
FindGeoClue2.cmake
FindHyphen.cmake
FindLibEpoxy.cmake
FindLibGBM.cmake
FindLibsecret.cmake
FindLibtasn1.cmake
FindLibxkbcommon.cmake
FindOpenGL.cmake
FindOpenGLES2.cmake
FindOpenWebRTC.cmake
FindWPEBackend-mesa.cmake
FindWPEBackend.cmake
FindWayland.cmake
FindWebP.cmake

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #2)
> I have no strong preference. Do want to write the patch, or shall I?
> 
> Are we going to have to change WEBP_LIBRARIES and WEBP_INCLUDE_DIRS to
> WebP_LIBRARIES and WebP_FOUND as well?

If we go to WebP way, yes

[Konstantin Tokarev]

(In reply to Konstantin Tokarev from comment #4)
> (In reply to Michael Catanzaro from comment #2)
> > I have no strong preference. Do want to write the patch, or shall I?
> > 
> > Are we going to have to change WEBP_LIBRARIES and WEBP_INCLUDE_DIRS to
> > WebP_LIBRARIES and WebP_FOUND as well?
> 
> If we go to WebP way, yes

Oops, actually no. It's just a good practise but not a requirement, and old modules like FindLibXml2 do this. What matters is that X in FindX.cmake and in find_package_handle_standard_args(X ...) have the same case.

[Konstantin Tokarev]

But we'll need to specify "wrong" variable names explicitly in each module which isn't doing this yet

[Michael Catanzaro]

I think I like CMake less and less the more I learn about it.

Konstantin, why don't you pick your preferred approach and we can just see how well it works out.

[Konstantin Tokarev]

Too much work to do by hand and I use only 2 of these modules, but I'll try to make a script for sed

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #7)
> I think I like CMake less and less the more I learn about it.

That's OK.

[Konstantin Tokarev]

Created attachment 317928 [details]
Patch

[Konstantin Tokarev]

Created attachment 317931 [details]
Patch

[Michael Catanzaro]

Comment on attachment 317931 [details]
Patch

Thanks, this could have been so much worse.

[Konstantin Tokarev]

Committed r220595: <http://trac.webkit.org/changeset/220595>