Bug 84895 - [CMake] Rewrite FindCairo.cmake.
: [CMake] Rewrite FindCairo.cmake.
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-25 15:18 PST by
Modified: 2012-05-03 17:41 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-25 15:18:40 PST
Created an attachment (id=138881) [details]
Patch

[CMake] Rewrite FindCairo.cmake.
------- Comment #1 From 2012-04-25 15:20:48 PST -------
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 From 2012-04-25 15:22:50 PST -------
Created an attachment (id=138882) [details]
Patch
------- Comment #3 From 2012-04-25 16:43:00 PST -------
(From update of attachment 138882 [details])
Attachment 138882 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12521653
------- Comment #4 From 2012-04-25 18:26:20 PST -------
Created an attachment (id=138918) [details]
Make the EWS and CMake 2.8.0 happy
------- Comment #5 From 2012-04-25 19:50:44 PST -------
(From update of attachment 138918 [details])
Attachment 138918 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12523775
------- Comment #6 From 2012-04-25 21:30:54 PST -------
Created an attachment (id=138930) [details]
Appease CMake 2.8.0 some more
------- Comment #7 From 2012-04-25 21:36:16 PST -------
(From update of attachment 138930 [details])
Attachment 138930 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12527780
------- Comment #8 From 2012-04-26 07:41:30 PST -------
Created an attachment (id=139000) [details]
Try to get some debugging information from the EWS
------- Comment #9 From 2012-04-26 08:43:40 PST -------
(From update of attachment 139000 [details])
Attachment 139000 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12525888
------- Comment #10 From 2012-04-26 13:57:04 PST -------
Created an attachment (id=139065) [details]
Add even more debugging output for the EWS
------- Comment #11 From 2012-04-26 16:54:12 PST -------
(From update of attachment 139065 [details])
Attachment 139065 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12558037
------- Comment #12 From 2012-04-26 17:11:56 PST -------
Created an attachment (id=139100) [details]
Retry after cleaning up CMakeCaches.txt in the EWS
------- Comment #13 From 2012-04-30 14:40:46 PST -------
(From update of attachment 139100 [details])
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 From 2012-04-30 14:41:23 PST -------
(From update of attachment 139100 [details])
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 From 2012-04-30 18:39:30 PST -------
Created an attachment (id=139569) [details]
Fix the ChangeLog nitpicks
------- Comment #16 From 2012-04-30 18:47:08 PST -------
(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 From 2012-04-30 21:04:10 PST -------
(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 From 2012-05-03 15:30:20 PST -------
(From update of attachment 139569 [details])
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 From 2012-05-03 17:37:11 PST -------
Created an attachment (id=140133) [details]
Patch for landing
------- Comment #20 From 2012-05-03 17:39:27 PST -------
Committed r116049: <http://trac.webkit.org/changeset/116049>
------- Comment #21 From 2012-05-03 17:41:13 PST -------
(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.