Created attachment 138881 [details] Patch [CMake] Rewrite FindCairo.cmake.
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.
Created attachment 138882 [details] Patch
Comment on attachment 138882 [details] Patch Attachment 138882 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12521653
Created attachment 138918 [details] Make the EWS and CMake 2.8.0 happy
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
Created attachment 138930 [details] Appease CMake 2.8.0 some more
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
Created attachment 139000 [details] Try to get some debugging information from the EWS
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
Created attachment 139065 [details] Add even more debugging output for the EWS
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
Created attachment 139100 [details] Retry after cleaning up CMakeCaches.txt in the EWS
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 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.
Created attachment 139569 [details] Fix the ChangeLog nitpicks
(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.
(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 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 :(
Created attachment 140133 [details] Patch for landing
Committed r116049: <http://trac.webkit.org/changeset/116049>
(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.