Bug 175427

Summary: [CMake] Fix broken use of REQUIRED with find modules
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Konstantin Tokarev <annulen>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bugs-noreply, clopez, mcatanzaro
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch mcatanzaro: review+

Description Michael Catanzaro 2017-08-10 08:09:17 PDT
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).
Comment 1 Konstantin Tokarev 2017-08-10 08:49:15 PDT
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
Comment 2 Michael Catanzaro 2017-08-10 10:24:58 PDT
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?
Comment 3 Konstantin Tokarev 2017-08-10 10:47:10 PDT
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
Comment 4 Konstantin Tokarev 2017-08-10 10:47:45 PDT
(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
Comment 5 Konstantin Tokarev 2017-08-10 11:02:15 PDT
(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.
Comment 6 Konstantin Tokarev 2017-08-10 11:05:17 PDT
But we'll need to specify "wrong" variable names explicitly in each module which isn't doing this yet
Comment 7 Michael Catanzaro 2017-08-10 11:16:12 PDT
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.
Comment 8 Konstantin Tokarev 2017-08-10 11:57:46 PDT
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
Comment 9 Konstantin Tokarev 2017-08-10 11:58:59 PDT
(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.
Comment 10 Konstantin Tokarev 2017-08-11 07:21:31 PDT
Created attachment 317928 [details]
Patch
Comment 11 Konstantin Tokarev 2017-08-11 08:34:01 PDT
Created attachment 317931 [details]
Patch
Comment 12 Michael Catanzaro 2017-08-11 08:44:05 PDT
Comment on attachment 317931 [details]
Patch

Thanks, this could have been so much worse.
Comment 13 Konstantin Tokarev 2017-08-11 09:39:17 PDT
Committed r220595: <http://trac.webkit.org/changeset/220595>