Bug 84895 - [CMake] Rewrite FindCairo.cmake.
: [CMake] Rewrite FindCairo.cmake.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Raphael Kubo da Costa (:rakuco)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 15:18 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2012-05-03 17:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (14.02 KB, patch)
2012-04-25 15:18 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (14.29 KB, patch)
2012-04-25 15:22 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Make the EWS and CMake 2.8.0 happy (14.40 KB, patch)
2012-04-25 18:26 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Appease CMake 2.8.0 some more (14.50 KB, patch)
2012-04-25 21:30 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Try to get some debugging information from the EWS (14.67 KB, patch)
2012-04-26 07:41 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Add even more debugging output for the EWS (14.92 KB, patch)
2012-04-26 13:57 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Retry after cleaning up CMakeCaches.txt in the EWS (14.50 KB, patch)
2012-04-26 17:11 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Fix the ChangeLog nitpicks (14.61 KB, patch)
2012-04-30 18:39 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch for landing (17.20 KB, patch)
2012-05-03 17:37 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2012-04-25 15:18:40 PDT
Created attachment 138881 [details]
Patch

[CMake] Rewrite FindCairo.cmake.
Comment 1 WebKit Review Bot 2012-04-25 15:20:48 PDT
Attachment 138881 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-25 15:22:50 PDT
Created attachment 138882 [details]
Patch
Comment 3 Gyuyoung Kim 2012-04-25 16:43:00 PDT
Comment on attachment 138882 [details]
Patch

Attachment 138882 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12521653
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-04-25 18:26:20 PDT
Created attachment 138918 [details]
Make the EWS and CMake 2.8.0 happy
Comment 5 Gyuyoung Kim 2012-04-25 19:50:44 PDT
Comment on attachment 138918 [details]
Make the EWS and CMake 2.8.0 happy

Attachment 138918 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12523775
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-04-25 21:30:54 PDT
Created attachment 138930 [details]
Appease CMake 2.8.0 some more
Comment 7 Gyuyoung Kim 2012-04-25 21:36:16 PDT
Comment on attachment 138930 [details]
Appease CMake 2.8.0 some more

Attachment 138930 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12527780
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-04-26 07:41:30 PDT
Created attachment 139000 [details]
Try to get some debugging information from the EWS
Comment 9 Gyuyoung Kim 2012-04-26 08:43:40 PDT
Comment on attachment 139000 [details]
Try to get some debugging information from the EWS

Attachment 139000 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12525888
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-04-26 13:57:04 PDT
Created attachment 139065 [details]
Add even more debugging output for the EWS
Comment 11 Gyuyoung Kim 2012-04-26 16:54:12 PDT
Comment on attachment 139065 [details]
Add even more debugging output for the EWS

Attachment 139065 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12558037
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-04-26 17:11:56 PDT
Created attachment 139100 [details]
Retry after cleaning up CMakeCaches.txt in the EWS
Comment 13 Daniel Bates 2012-04-30 14:40:46 PDT
Comment on attachment 139100 [details]
Retry after cleaning up CMakeCaches.txt in the EWS

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

> Source/WebCore/ChangeLog:9
> +        introduces a dependency on pkg-config that could be avoided), used the

Nit: introduces => introduced

> Source/WebCore/ChangeLog:11
> +        LibFindMacros code that we should probably remove in the future and
> +        did not use the FindPackageHandleStandardArguments module.

How did you come to the decision to use LibFindMacros instead of FindPackageHandleStandardArguments? Is the module FindPackageHandleStandardArguments in our repository?

> Source/WebCore/ChangeLog:15
> +        The only downside is that FPHSA sets <Upper-cased name>_FOUND instead

Nit: "Upper-cased name" isn't in uppercase. It's in title case. And uppercase is one word. So,  "<Upper-cased name>_FOUND" => "<UPPERCASED_NAME>_FOUND".

> Source/WebCore/ChangeLog:16
> +        of <Name>_FOUND, and to keep things consistency Cairo_LIBRARIES and

Nit: consistency => consistent

> Source/WebCore/ChangeLog:19
> +        No new tests, buildsystem change.

Nit: buildsystem => build system

> Source/WebKit/ChangeLog:9
> +        introduces a dependency on pkg-config that could be avoided), used the

Nit: introduces => introduced

> Source/WebKit/ChangeLog:16
> +        of <Name>_FOUND, and to keep things consistency Cairo_LIBRARIES and

Nit: consistency => consistent

> Source/cmake/FindCairo.cmake:32
> +FIND_PACKAGE(PkgConfig)
> +PKG_CHECK_MODULES(PC_CAIRO cairo) # FIXME: After we require CMake 2.8.2 we can pass QUIET to this call.

I thought the purpose of this patch was to remove the dependency on pkg-config?

(The FIXME only states that as of CMake 2.8.2 we can pass argument QUIET to this function so that no status message will be printed, but it doesn't talk about removing this code.)

> Source/cmake/FindCairo.cmake:78
> +SET(VERSION_OK TRUE)
> +IF (Cairo_FIND_VERSION)
> +    IF (Cairo_FIND_VERSION_EXACT)
> +        IF ("${Cairo_FIND_VERSION}" VERSION_EQUAL "${CAIRO_VERSION}")
> +            # FIXME: Use IF (NOT ...) with CMake 2.8.2+ to get rid of the ELSE block
> +        ELSE ()
> +            SET(VERSION_OK FALSE)
> +        ENDIF ()
> +    ELSE ()
> +        IF ("${Cairo_FIND_VERSION}" VERSION_GREATER "${CAIRO_VERSION}")
> +            SET(VERSION_OK FALSE)
> +        ENDIF ()
> +    ENDIF ()
> +ENDIF ()

I don't understand the purpose of specially handling the Cairo_FIND_VERSION_EXACT case. It seems sufficient to write this in terms of VERSION_GREATER and VERSION_LESS.
Comment 14 Daniel Bates 2012-04-30 14:41:23 PDT
Comment on attachment 139100 [details]
Retry after cleaning up CMakeCaches.txt in the EWS

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

>> Source/WebCore/ChangeLog:11
>> +        did not use the FindPackageHandleStandardArguments module.
> 
> How did you come to the decision to use LibFindMacros instead of FindPackageHandleStandardArguments? Is the module FindPackageHandleStandardArguments in our repository?

Disregard this remark.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-04-30 18:39:30 PDT
Created attachment 139569 [details]
Fix the ChangeLog nitpicks
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-04-30 18:47:08 PDT
(In reply to comment #13)
> > Source/cmake/FindCairo.cmake:32
> > +FIND_PACKAGE(PkgConfig)
> > +PKG_CHECK_MODULES(PC_CAIRO cairo) # FIXME: After we require CMake 2.8.2 we can pass QUIET to this call.
> 
> I thought the purpose of this patch was to remove the dependency on pkg-config?
> 
> (The FIXME only states that as of CMake 2.8.2 we can pass argument QUIET to this function so that no status message will be printed, but it doesn't talk about removing this code.)

The patch does remove the dependency on both pkg-config and LibFindMacros -- the former is now optional (ie. the Find module works if it not present) and the latter is not used anymore.

> > Source/cmake/FindCairo.cmake:78
> > +SET(VERSION_OK TRUE)
> > +IF (Cairo_FIND_VERSION)
> > +    IF (Cairo_FIND_VERSION_EXACT)
> > +        IF ("${Cairo_FIND_VERSION}" VERSION_EQUAL "${CAIRO_VERSION}")
> > +            # FIXME: Use IF (NOT ...) with CMake 2.8.2+ to get rid of the ELSE block
> > +        ELSE ()
> > +            SET(VERSION_OK FALSE)
> > +        ENDIF ()
> > +    ELSE ()
> > +        IF ("${Cairo_FIND_VERSION}" VERSION_GREATER "${CAIRO_VERSION}")
> > +            SET(VERSION_OK FALSE)
> > +        ENDIF ()
> > +    ENDIF ()
> > +ENDIF ()
> 
> I don't understand the purpose of specially handling the Cairo_FIND_VERSION_EXACT case. It seems sufficient to write this in terms of VERSION_GREATER and VERSION_LESS.

I've used (and removed unnecessary bits from) the code in the second form of the FindPackageHandleStandardArguments that accepts the VERSION_VAR argument and is present in later CMake versions. I just tried to deviate as little as possible from upstream here.
Comment 17 Raphael Kubo da Costa (:rakuco) 2012-04-30 21:04:10 PDT
(In reply to comment #16)
> (In reply to comment #13)
> > > Source/cmake/FindCairo.cmake:32
> > > +FIND_PACKAGE(PkgConfig)
> > > +PKG_CHECK_MODULES(PC_CAIRO cairo) # FIXME: After we require CMake 2.8.2 we can pass QUIET to this call.
> > 
> > I thought the purpose of this patch was to remove the dependency on pkg-config?
> > 
> > (The FIXME only states that as of CMake 2.8.2 we can pass argument QUIET to this function so that no status message will be printed, but it doesn't talk about removing this code.)
> 
> The patch does remove the dependency on both pkg-config and LibFindMacros -- the former is now optional (ie. the Find module works if it not present) and the latter is not used anymore.

Oh, and it also removes the implicit dependency on Freetype, which is often detected the wrong way, causing some CMake warnings about dependencies being shadowed.
Comment 18 Daniel Bates 2012-05-03 15:30:20 PDT
Comment on attachment 139569 [details]
Fix the ChangeLog nitpicks

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

> Source/WebCore/ChangeLog:14
> +        Change all that by rewriting the module.

It's unclear if "change all that" implies the negation of the "old approach" or whether the "new approach" is something entirely different.

For your consideration, I suggest writing a short summary of the changes so as to make it straightforward for someone to understand the "new approach" without having to read the patch.

> Source/cmake/FindCairo.cmake:51
> +        STRING(REGEX MATCH "#define +CAIRO_VERSION_MAJOR +([0-9]+)" _dummy "${CAIRO_VERSION_CONTENT}")

Would it make sense to strengthen this regular expression by matching the start and/or end of the line?

> Source/cmake/FindCairo.cmake:54
> +        STRING(REGEX MATCH "#define +CAIRO_VERSION_MINOR +([0-9]+)" _dummy "${CAIRO_VERSION_CONTENT}")

Ditto.

> Source/cmake/FindCairo.cmake:57
> +        STRING(REGEX MATCH "#define +CAIRO_VERSION_MICRO +([0-9]+)" _dummy "${CAIRO_VERSION_CONTENT}")

Ditto.

> Source/cmake/FindCairo.cmake:78
> +IF (Cairo_FIND_VERSION)
> +    IF (Cairo_FIND_VERSION_EXACT)
> +        IF ("${Cairo_FIND_VERSION}" VERSION_EQUAL "${CAIRO_VERSION}")
> +            # FIXME: Use IF (NOT ...) with CMake 2.8.2+ to get rid of the ELSE block
> +        ELSE ()
> +            SET(VERSION_OK FALSE)
> +        ENDIF ()
> +    ELSE ()
> +        IF ("${Cairo_FIND_VERSION}" VERSION_GREATER "${CAIRO_VERSION}")
> +            SET(VERSION_OK FALSE)
> +        ENDIF ()
> +    ENDIF ()
> +ENDIF ()

I wish there was a way to simplify this :(
Comment 19 Raphael Kubo da Costa (:rakuco) 2012-05-03 17:37:11 PDT
Created attachment 140133 [details]
Patch for landing
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-05-03 17:39:27 PDT
Committed r116049: <http://trac.webkit.org/changeset/116049>
Comment 21 Raphael Kubo da Costa (:rakuco) 2012-05-03 17:41:13 PDT
(In reply to comment #18)
> > Source/WebCore/ChangeLog:14
> > +        Change all that by rewriting the module.
> 
> It's unclear if "change all that" implies the negation of the "old approach" or whether the "new approach" is something entirely different.
> 
> For your consideration, I suggest writing a short summary of the changes so as to make it straightforward for someone to understand the "new approach" without having to read the patch.

Done.

> > Source/cmake/FindCairo.cmake:51
> > +        STRING(REGEX MATCH "#define +CAIRO_VERSION_MAJOR +([0-9]+)" _dummy "${CAIRO_VERSION_CONTENT}")
> 
> Would it make sense to strengthen this regular expression by matching the start and/or end of the line?

I can't do that unless I iterate through all the lines, otherwise the file's contents is just one big string.

> > Source/cmake/FindCairo.cmake:78
> > +IF (Cairo_FIND_VERSION)
> > +    IF (Cairo_FIND_VERSION_EXACT)
> > +        IF ("${Cairo_FIND_VERSION}" VERSION_EQUAL "${CAIRO_VERSION}")
> > +            # FIXME: Use IF (NOT ...) with CMake 2.8.2+ to get rid of the ELSE block
> > +        ELSE ()
> > +            SET(VERSION_OK FALSE)
> > +        ENDIF ()
> > +    ELSE ()
> > +        IF ("${Cairo_FIND_VERSION}" VERSION_GREATER "${CAIRO_VERSION}")
> > +            SET(VERSION_OK FALSE)
> > +        ENDIF ()
> > +    ENDIF ()
> > +ENDIF ()
> 
> I wish there was a way to simplify this :(

I agree; it's all going away once we depend on CMake 2.8.3+, though.