Summary: | [CMake] Fix broken use of REQUIRED with find modules | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
Component: | WebKitGTK | Assignee: | Konstantin Tokarev <annulen> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, bugs-noreply, clopez, mcatanzaro | ||||||
Priority: | P2 | ||||||||
Version: | Other | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-08-10 08:09:17 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 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? 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 (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 (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. But we'll need to specify "wrong" variable names explicitly in each module which isn't doing this yet 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. 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 (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. Created attachment 317928 [details]
Patch
Created attachment 317931 [details]
Patch
Comment on attachment 317931 [details]
Patch
Thanks, this could have been so much worse.
Committed r220595: <http://trac.webkit.org/changeset/220595> |