Bug 124555 - [EFL] Use Config mode of find_package for EFL 1.8
Summary: [EFL] Use Config mode of find_package for EFL 1.8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 124628 124641
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-18 18:52 PST by Ryuan Choi
Modified: 2013-11-25 18:14 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2013-11-18 19:03 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2013-11-18 21:24 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (3.55 KB, patch)
2013-11-19 03:29 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (6.47 KB, patch)
2013-11-20 17:00 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2013-11-18 18:52:16 PST
EFL 1.8 changed VERSION macro so it's difficult to use tricky approach which parses header file to know the VERSION.
Instead, EFL 1.8 supports Config mode of find_package using XXXConfig.cmake such as EinaConfig.cmake

So, we can use Config mode of find_package when we found Eo (which is introduced since EFL 1.8).
Comment 1 Ryuan Choi 2013-11-18 19:03:04 PST
Created attachment 217263 [details]
Patch
Comment 2 Ryuan Choi 2013-11-18 21:24:29 PST
Created attachment 217268 [details]
Patch
Comment 3 Gyuyoung Kim 2013-11-18 21:34:34 PST
Comment on attachment 217268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217268&action=review

Looks fine except for some English nits.

> Source/cmake/OptionsEfl.cmake:130
> +    # EFL 1.8 provide XXXConfig.cmake

- provide -> provides ? 
- XXXConfig -> FooConfig.

> Tools/ChangeLog:8
> +        * MiniBrowser/efl/CMakeLists.txt: Introduced Config mode to find elementary.

Config mode -> a config mode ?

> ChangeLog:12
> +        This patch try to use Config mode if it is available after checked Eo.

try -> tries ? 
after checked -> after checking ?
Comment 4 Ryuan Choi 2013-11-19 03:29:59 PST
Created attachment 217285 [details]
Patch
Comment 5 Ryuan Choi 2013-11-19 03:30:27 PST
(In reply to comment #3)
> (From update of attachment 217268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217268&action=review
> 
> Looks fine except for some English nits.
> 
> > Source/cmake/OptionsEfl.cmake:130
> > +    # EFL 1.8 provide XXXConfig.cmake
> 
> - provide -> provides ? 
> - XXXConfig -> FooConfig.
> 
> > Tools/ChangeLog:8
> > +        * MiniBrowser/efl/CMakeLists.txt: Introduced Config mode to find elementary.
> 
> Config mode -> a config mode ?
> 
> > ChangeLog:12
> > +        This patch try to use Config mode if it is available after checked Eo.
> 
> try -> tries ? 
> after checked -> after checking ?

Thanks, I fixed.
Comment 6 WebKit Commit Bot 2013-11-19 04:36:16 PST
Comment on attachment 217285 [details]
Patch

Clearing flags on attachment: 217285

Committed r159496: <http://trac.webkit.org/changeset/159496>
Comment 7 WebKit Commit Bot 2013-11-19 04:36:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Seokju Kwon 2013-11-19 17:48:17 PST
It seems like 'CONFIG' option of find_package() is supported on cmake 3.8.8 or later.
But the minimum required version of cmake is 2.8.3.

Error : 
CMake Error at Source/cmake/OptionsEfl.cmake:126 (find_package):
  find_package called with invalid argument "CONFIG"
Comment 9 WebKit Commit Bot 2013-11-20 00:10:56 PST
Re-opened since this is blocked by bug 124641
Comment 10 Csaba Osztrogonác 2013-11-20 03:25:06 PST
Ubuntu 12.04 (the latest LTS) is shipped with cmake 2.8.7,
I don't think if we should drop supporting this release.
Comment 11 Gyuyoung Kim 2013-11-20 03:34:44 PST
(In reply to comment #10)
> Ubuntu 12.04 (the latest LTS) is shipped with cmake 2.8.7,
> I don't think if we should drop supporting this release.

Ryuan will find a better way to support this mode.
Comment 12 Ryuan Choi 2013-11-20 17:00:33 PST
Created attachment 217495 [details]
Patch
Comment 13 Ryuan Choi 2013-11-20 17:04:49 PST
(In reply to comment #8)
> It seems like 'CONFIG' option of find_package() is supported on cmake 3.8.8 or later.
> But the minimum required version of cmake is 2.8.3.
> 
> Error : 
> CMake Error at Source/cmake/OptionsEfl.cmake:126 (find_package):
>   find_package called with invalid argument "CONFIG"

Seokju, could you check with newer patch.
I believe that it will be solved.

(In reply to comment #11)
> (In reply to comment #10)
> > Ubuntu 12.04 (the latest LTS) is shipped with cmake 2.8.7,
> > I don't think if we should drop supporting this release.
> 
> Ryuan will find a better way to support this mode.

Sure, I don't want to drop them.

Instead, I changed little bit.

Eo will not be checked with config mode by removing legacy FindEo.cmake.
And added QUIET not to print warning.
To support EFL 1.8 with cmake lower than 2.8.8, I supposed to use legacy FindFoo.cmake without version requirement because Eo will guarantee the version.
Comment 14 Seokju Kwon 2013-11-25 17:12:42 PST
LGTM with EFL 1.7
Thanks.
Comment 15 Gyuyoung Kim 2013-11-25 17:36:50 PST
Comment on attachment 217495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217495&action=review

Looks fine.

> Source/cmake/OptionsEfl.cmake:136
> +    if (NOT (CMAKE_VERSION VERSION_LESS 2.8.8))

Isn't there CMAKE_VERSION_VERSION_MORE or similar thing ? It looks this is double *not not* condition. If there is no similar thing, this is fine.
Comment 16 Ryuan Choi 2013-11-25 17:47:15 PST
(In reply to comment #15)
> (From update of attachment 217495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217495&action=review
> 
> Looks fine.
> 
> > Source/cmake/OptionsEfl.cmake:136
> > +    if (NOT (CMAKE_VERSION VERSION_LESS 2.8.8))
> 
> Isn't there CMAKE_VERSION_VERSION_MORE or similar thing ? It looks this is double *not not* condition. If there is no similar thing, this is fine.

Unfortunately cmake only have VERSION_LESS, VERSION_EQUAL, VERSION_GREATER.
So, below is another option but also complicated.
if ((CMAKE_VERSION VERSION_GREATER 2.8.8) OR (CMAKE_VERSION VERSION_EQUAL 2.8.8))

http://stackoverflow.com/questions/16667017/cmake-express-the-greater-or-equal-statement
Comment 17 WebKit Commit Bot 2013-11-25 18:13:56 PST
Comment on attachment 217495 [details]
Patch

Clearing flags on attachment: 217495

Committed r159776: <http://trac.webkit.org/changeset/159776>
Comment 18 WebKit Commit Bot 2013-11-25 18:14:01 PST
All reviewed patches have been landed.  Closing bug.