|Summary:||[CMake] Fix broken use of REQUIRED with find modules|
|Product:||WebKit||Reporter:||Michael Catanzaro <mcatanzaro>|
|Component:||WebKitGTK||Assignee:||Konstantin Tokarev <annulen>|
|Severity:||Normal||CC:||annulen, bugs-noreply, clopez, mcatanzaro|
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 12 Michael Catanzaro 2017-08-11 08:44:05 PDT
Comment on attachment 317931 [details] Patch Thanks, this could have been so much worse.