Summary: | [CMake] Remove support for cmake version < 2.8.3 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | afonso.costa, commit-queue, gyuyoung.kim, gyuyoung.kim, ossy, rakuco, ryuan.choi | ||||||||
Priority: | P2 | Keywords: | EasyFix | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Laszlo Gombos
2012-09-24 21:00:29 PDT
Note that FindGStreamer.cmake and FindCairo.cmake can be now simplified assuming cmake >=2.8.3. Created attachment 214018 [details]
Patch
Comment on attachment 214018 [details]
Patch
LGTM
Comment on attachment 214018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214018&action=review > ChangeLog:3 > + [CMAKE] Update code to take advantage of CMake version 2.8.3+. Hmm, Bug title should be same with Bug 97516 although this looks more informative. > Source/cmake/FindCairo.cmake:-78 > -endif () I am not sure. With this patch, find_package(Cairo 999.10.2 REQUIRED) is fine. Comment on attachment 214018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214018&action=review >> Source/cmake/FindCairo.cmake:-78 >> -endif () > > I am not sure. > With this patch, find_package(Cairo 999.10.2 REQUIRED) is fine. This review comment isn't clear for me. Currently we use "Cairo 1.10.2" for EFL and GTK ports. Do you mean this patch will not support 999.10.2 version style ? (In reply to comment #5) > (From update of attachment 214018 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214018&action=review > > >> Source/cmake/FindCairo.cmake:-78 > >> -endif () > > > > I am not sure. > > With this patch, find_package(Cairo 999.10.2 REQUIRED) is fine. > > This review comment isn't clear for me. Currently we use "Cairo 1.10.2" for EFL and GTK ports. Do you mean this patch will not support 999.10.2 version style ? I mean that FindCairo.cmake will not complain with this pacth although find_package requests higher version than current version which system have. 999.10.2 is just one of example, which is higher version than currently installed. I think that FindCairo.cmake should raise assert when find_package request higer version. Comment on attachment 214018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214018&action=review >>>> Source/cmake/FindCairo.cmake:-78 >>>> -endif () >>> >>> I am not sure. >>> With this patch, find_package(Cairo 999.10.2 REQUIRED) is fine. >> >> This review comment isn't clear for me. Currently we use "Cairo 1.10.2" for EFL and GTK ports. Do you mean this patch will not support 999.10.2 version style ? > > I mean that FindCairo.cmake will not complain with this pacth although find_package requests higher version than current version which system have. > > 999.10.2 is just one of example, which is higher version than currently installed. > > I think that FindCairo.cmake should raise assert when find_package request higer version. Ok, I see. This patch can't generate compilation fail when higher version is required. This patch should check if higher version is required. (In reply to comment #7) > (From update of attachment 214018 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214018&action=review > > >>>> Source/cmake/FindCairo.cmake:-78 > >>>> -endif () > >>> > >>> I am not sure. > >>> With this patch, find_package(Cairo 999.10.2 REQUIRED) is fine. > >> > >> This review comment isn't clear for me. Currently we use "Cairo 1.10.2" for EFL and GTK ports. Do you mean this patch will not support 999.10.2 version style ? > > > > I mean that FindCairo.cmake will not complain with this pacth although find_package requests higher version than current version which system have. > > > > 999.10.2 is just one of example, which is higher version than currently installed. > > > > I think that FindCairo.cmake should raise assert when find_package request higer version. > > Ok, I see. This patch can't generate compilation fail when higher version is required. This patch should check if higher version is required. Hi Ryuan and Gyuyoung, thanks for review. I've updated the patch according to your suggestions. But, I have one doubt yet: are the changes in the other files ok for you or do I need to make the check also? Created attachment 214171 [details]
Patch v2
The initial patch was updated:
- Raise a message when the required version is higher than found version.
- I've removed the changes in the files used by GTK build (it seems that GTK is using autotools instead of CMake).
Comment on attachment 214171 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=214171&action=review > Source/cmake/FindGStreamer.cmake:-108 > -endif () Why didn't you adjust it to gstreamer ? (In reply to comment #10) > (From update of attachment 214171 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214171&action=review > > > Source/cmake/FindGStreamer.cmake:-108 > > -endif () > > Why didn't you adjust it to gstreamer ? Hi Gyuyoung, because if I change the gstreamer's required version in find_package a message is already raised. Maybe this behaviour is related to REQUIRED_VARS. But, I can put the check in FindGStreamer also, no problem. Created attachment 214255 [details]
Patch v3
Checking if the GStreamer required version is higher than found version (same way that is being used in FindCairo.cmake)
Comment on attachment 214255 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=214255&action=review I checked this patch can check if higher version is required. However, Ryuan might want to have a final look before landing. > ChangeLog:3 > + [CMAKE] Update code to take advantage of CMake version 2.8.3+. Please sync with this bug title before landing. It looks this one is better than this bug title. Any update ? Ryuan ? Comment on attachment 214255 [details]
Patch v3
Looks fine to me.
Comment on attachment 214255 [details] Patch v3 Clearing flags on attachment: 214255 Committed r157562: <http://trac.webkit.org/changeset/157562> All reviewed patches have been landed. Closing bug. |