Bug 84895 – [CMake] Rewrite FindCairo.cmake.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Raphael Kubo da Costa (:rakuco)]

Created attachment 138881 [details]
Patch

[CMake] Rewrite FindCairo.cmake.

[WebKit Review Bot]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 138882 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 138882 [details]
Patch

Attachment 138882 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12521653

[Raphael Kubo da Costa (:rakuco)]

Created attachment 138918 [details]
Make the EWS and CMake 2.8.0 happy

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 138930 [details]
Appease CMake 2.8.0 some more

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139000 [details]
Try to get some debugging information from the EWS

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139065 [details]
Add even more debugging output for the EWS

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139100 [details]
Retry after cleaning up CMakeCaches.txt in the EWS

[Daniel Bates]

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.

[Daniel Bates]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139569 [details]
Fix the ChangeLog nitpicks

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Daniel Bates]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 140133 [details]
Patch for landing

[Raphael Kubo da Costa (:rakuco)]

Committed r116049: <http://trac.webkit.org/changeset/116049>

[Raphael Kubo da Costa (:rakuco)]

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