Bug 97516 – [CMake] Remove support for cmake version < 2.8.3

• Component: Platform
• Status: RESOLVED FIXED

[Laszlo Gombos]

http://trac.webkit.org/changeset/125228 made cmake version 2.8.3 a requirement but did not removed some code that supports cmake version < 2.8.3 - according to https://bugs.webkit.org/show_bug.cgi?id=84895#c21. This bug is to remove the remaining dead code/refactor existing code to take advantage of cmake version 2.8.3+

[Laszlo Gombos]

Note that FindGStreamer.cmake and FindCairo.cmake can be now simplified assuming cmake >=2.8.3.

[Afonso Costa]

Created attachment 214018 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 214018 [details]
Patch

LGTM

[Ryuan Choi]

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.

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Afonso Costa]

(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?

[Afonso Costa]

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).

[Gyuyoung Kim]

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 ?

[Afonso Costa]

(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.

[Afonso Costa]

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)

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Any update ? Ryuan ?

[Ryuan Choi]

Comment on attachment 214255 [details]
Patch v3

Looks fine to me.

[WebKit Commit Bot]

Comment on attachment 214255 [details]
Patch v3

Clearing flags on attachment: 214255

Committed r157562: <http://trac.webkit.org/changeset/157562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.