Re: [cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
On 07/14/2016 06:31 PM, Ben Campbell wrote: > new patch attached! Thanks, looks good! Applied and merged to `next` for testing: FindGIF: Detect version from GIF 4.1.6 and above https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=0a6c227d Once this is merged to `master` the issue should close automatically. -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
On 15/07/16 06:47, Brad King wrote: On 07/13/2016 06:59 PM, Ben Campbell wrote: I've revised my patch to: Thanks. Please use `git format-patch` and then attach it. The inline patch was corrupted and does not apply. Oop - new patch attached! 1/1 Test #268: CMakeOnly.AllFindModules .***Failed8.07 sec It doesn't expect any specific environment. It is just testing that all find modules can complete whether the packages are found or not. The "Failed to find" lines are not a problem. Other output may show a real "CMake Error" though. OK - picking my way through it now - will split into a separate thread. Thanks, Ben. >From a89a83c5b02bc91d1beb01d2c69a44c3de1eab35 Mon Sep 17 00:00:00 2001 From: Ben CampbellDate: Wed, 13 Jul 2016 15:05:47 +1200 Subject: [PATCH] Improve FindGIF version detection (fix for issue #16196) --- Modules/FindGIF.cmake | 46 +++--- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/Modules/FindGIF.cmake b/Modules/FindGIF.cmake index 7bbb8cf..d657d83 100644 --- a/Modules/FindGIF.cmake +++ b/Modules/FindGIF.cmake @@ -2,12 +2,18 @@ # FindGIF # --- # +# This finds the GIF library (giflib) # +# The module defines the following variables: # -# This module searches giflib and defines GIF_LIBRARIES - libraries to -# link to in order to use GIF GIF_FOUND, if false, do not try to link -# GIF_INCLUDE_DIR, where to find the headers GIF_VERSION, reports either -# version 4 or 3 (for everything before version 4) +# ``GIF_FOUND`` +# True if giflib was found +# ``GIF_LIBRARIES`` +# Libraries to link to in order to use giflib +# ``GIF_INCLUDE_DIR`` +# where to find the headers +# ``GIF_VERSION`` +# 3, 4 or a full version string (eg 5.1.4) for versions >= 4.1.6 # # The minimum required version of giflib can be specified using the # standard syntax, e.g. find_package(GIF 4) @@ -29,7 +35,7 @@ # License text for the above reference.) # Created by Eric Wing. -# Modifications by Alexander Neundorf +# Modifications by Alexander Neundorf, Ben Campbell find_path(GIF_INCLUDE_DIR gif_lib.h HINTS @@ -61,22 +67,40 @@ set(GIF_LIBRARIES ${GIF_LIBRARY}) # to be always " Version 2.0, " in versions 3.x of giflib. # In version 4 the member UserData was added to GifFileType, so we check for this # one. -# http://giflib.sourcearchive.com/documentation/4.1.4/files.html +# Versions after 4.1.6 define GIFLIB_MAJOR, GIFLIB_MINOR, and GIFLIB_RELEASE +# see http://giflib.sourceforge.net/gif_lib.html#compatibility if(GIF_INCLUDE_DIR) include(${CMAKE_CURRENT_LIST_DIR}/CMakePushCheckState.cmake) include(${CMAKE_CURRENT_LIST_DIR}/CheckStructHasMember.cmake) CMAKE_PUSH_CHECK_STATE() set(CMAKE_REQUIRED_QUIET ${GIF_FIND_QUIETLY}) - set(GIF_VERSION 3) set(CMAKE_REQUIRED_INCLUDES "${GIF_INCLUDE_DIR}") - CHECK_STRUCT_HAS_MEMBER(GifFileType UserData gif_lib.h GIF_GifFileType_UserData ) - if(GIF_GifFileType_UserData) -set(GIF_VERSION 4) + + # Check for the specific version defines (>=4.1.6 only) + file(STRINGS ${GIF_INCLUDE_DIR}/gif_lib.h _GIF_DEFS REGEX "^[ \t]*#define[ \t]+GIFLIB_(MAJOR|MINOR|RELEASE)") + if(_GIF_DEFS) +# yay - got exact version info +string(REGEX REPLACE ".*GIFLIB_MAJOR ([0-9]+).*" "\\1" _GIF_MAJ ${_GIF_DEFS}) +string(REGEX REPLACE ".*GIFLIB_MINOR ([0-9]+).*" "\\1" _GIF_MIN ${_GIF_DEFS}) +string(REGEX REPLACE ".*GIFLIB_RELEASE ([0-9]+).*" "\\1" _GIF_REL ${_GIF_DEFS}) +set(GIF_VERSION ${_GIF_MAJ}.${_GIF_MIN}.${_GIF_REL}) + else() +# use UserData field to sniff version instead +CHECK_STRUCT_HAS_MEMBER(GifFileType UserData gif_lib.h GIF_GifFileType_UserData ) +if(GIF_GifFileType_UserData) + set(GIF_VERSION 4) +else() + set(GIF_VERSION 3) +endif() endif() + + unset(_GIF_MAJ) + unset(_GIF_MIN) + unset(_GIF_REL) + unset(_GIF_DEFS) CMAKE_POP_CHECK_STATE() endif() - # handle the QUIETLY and REQUIRED arguments and set GIF_FOUND to TRUE if # all listed variables are TRUE include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake) -- 2.7.4 -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
On 07/13/2016 06:59 PM, Ben Campbell wrote: > I've revised my patch to: Thanks. Please use `git format-patch` and then attach it. The inline patch was corrupted and does not apply. > 1/1 Test #268: CMakeOnly.AllFindModules .***Failed8.07 sec > > Not quite sure what the environment the tests expect - is it just a case > that I've not got all the expected libraries installed? It doesn't expect any specific environment. It is just testing that all find modules can complete whether the packages are found or not. The "Failed to find" lines are not a problem. Other output may show a real "CMake Error" though. -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
On 14/07/16 01:52, Brad King wrote: On 07/12/2016 11:16 PM, Ben Campbell wrote: A fix for https://gitlab.kitware.com/cmake/cmake/issues/16196 Thanks! Here are some comments. [snip] Cool - very helpful! I've revised my patch to: - only scan the headerfile once - update the docs in the comment block - unset the scratch variables If the version is now expected to work please also update `Tests/CMakeOnly/AllFindModules/CMakeLists.txt` to check that a version number is extracted. In a CMake build tree run `ctest -R CMakeOnly.AllFindModules -V` to run the test. The existing GIF test looks like it'll keep working fine, so I've not fiddled with the tests at all. Actually, the tests didn't complete on my machine anyway. Looking at the output, I'm pretty sure the GIF part worked. Here's a very cut-down log of the test output (running the latest cmake from git, in situ): $ bin/ctest -R CMakeOnly.AllFindModules -V | grep -i fail Guessing configuration NoConfig 268: -- Trying to find DCMTK expecting DCMTKConfig.cmake - failed 268: -- Failed to find all ICU components (missing: ICU_LIBRARY) (found version "55.1") 268: -- Failed to find all Ice components (missing: Ice_SLICE2CPP_EXECUTABLE Ice_INCLUDE_DIR Ice_SLICE_DIR Ice_LIBRARY) 268: -- Failed to find XercesC (missing: XercesC_LIBRARY XercesC_INCLUDE_DIR XercesC_VERSION) 268: -- Failed to find XalanC (missing: XalanC_LIBRARY XalanC_INCLUDE_DIR XalanC_VERSION XercesC_FOUND) 268: -- Failed to find XercesC (missing: XercesC_LIBRARY XercesC_INCLUDE_DIR XercesC_VERSION) 268: CMake failed to configure AllFindModules 1/1 Test #268: CMakeOnly.AllFindModules .***Failed8.07 sec 0% tests passed, 1 tests failed out of 1 The following tests FAILED: 268 - CMakeOnly.AllFindModules (Failed) Errors while running CTest Not quite sure what the environment the tests expect - is it just a case that I've not got all the expected libraries installed? Ben. --- Modules/FindGIF.cmake | 46 +++--- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/Modules/FindGIF.cmake b/Modules/FindGIF.cmake index 7bbb8cf..d657d83 100644 --- a/Modules/FindGIF.cmake +++ b/Modules/FindGIF.cmake @@ -2,12 +2,18 @@ # FindGIF # --- # +# This finds the GIF library (giflib) # +# The module defines the following variables: # -# This module searches giflib and defines GIF_LIBRARIES - libraries to -# link to in order to use GIF GIF_FOUND, if false, do not try to link -# GIF_INCLUDE_DIR, where to find the headers GIF_VERSION, reports either -# version 4 or 3 (for everything before version 4) +# ``GIF_FOUND`` +# True if giflib was found +# ``GIF_LIBRARIES`` +# Libraries to link to in order to use giflib +# ``GIF_INCLUDE_DIR`` +# where to find the headers +# ``GIF_VERSION`` +# 3, 4 or a full version string (eg 5.1.4) for versions >= 4.1.6 # # The minimum required version of giflib can be specified using the # standard syntax, e.g. find_package(GIF 4) @@ -29,7 +35,7 @@ # License text for the above reference.) # Created by Eric Wing. -# Modifications by Alexander Neundorf +# Modifications by Alexander Neundorf, Ben Campbell find_path(GIF_INCLUDE_DIR gif_lib.h HINTS @@ -61,22 +67,40 @@ set(GIF_LIBRARIES ${GIF_LIBRARY}) # to be always " Version 2.0, " in versions 3.x of giflib. # In version 4 the member UserData was added to GifFileType, so we check for this # one. -# http://giflib.sourcearchive.com/documentation/4.1.4/files.html +# Versions after 4.1.6 define GIFLIB_MAJOR, GIFLIB_MINOR, and GIFLIB_RELEASE +# see http://giflib.sourceforge.net/gif_lib.html#compatibility if(GIF_INCLUDE_DIR) include(${CMAKE_CURRENT_LIST_DIR}/CMakePushCheckState.cmake) include(${CMAKE_CURRENT_LIST_DIR}/CheckStructHasMember.cmake) CMAKE_PUSH_CHECK_STATE() set(CMAKE_REQUIRED_QUIET ${GIF_FIND_QUIETLY}) - set(GIF_VERSION 3) set(CMAKE_REQUIRED_INCLUDES "${GIF_INCLUDE_DIR}") - CHECK_STRUCT_HAS_MEMBER(GifFileType UserData gif_lib.h GIF_GifFileType_UserData ) - if(GIF_GifFileType_UserData) -set(GIF_VERSION 4) + + # Check for the specific version defines (>=4.1.6 only) + file(STRINGS ${GIF_INCLUDE_DIR}/gif_lib.h _GIF_DEFS REGEX "^[ \t]*#define[ \t]+GIFLIB_(MAJOR|MINOR|RELEASE)") + if(_GIF_DEFS) +# yay - got exact version info +string(REGEX REPLACE ".*GIFLIB_MAJOR ([0-9]+).*" "\\1" _GIF_MAJ ${_GIF_DEFS}) +string(REGEX REPLACE ".*GIFLIB_MINOR ([0-9]+).*" "\\1" _GIF_MIN ${_GIF_DEFS}) +string(REGEX REPLACE ".*GIFLIB_RELEASE ([0-9]+).*" "\\1" _GIF_REL ${_GIF_DEFS}) +set(GIF_VERSION ${_GIF_MAJ}.${_GIF_MIN}.${_GIF_REL}) + else() +# use UserData field to sniff version instead +CHECK_STRUCT_HAS_MEMBER(GifFileType UserData gif_lib.h GIF_GifFileType_UserData ) +if(GIF_GifFileType_UserData) + set(GIF_VERSION 4) +else() + set(GIF_VERSION 3) +endif() endif() + + unset(_GIF_MAJ) + unset(_GIF_MIN) + unset(_GIF_REL) +
Re: [cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
On 07/12/2016 11:16 PM, Ben Campbell wrote: > A fix for https://gitlab.kitware.com/cmake/cmake/issues/16196 Thanks! Here are some comments. > the pairs of file()/string() > commands seem a bit convoluted for extracting strings out of the header > file - is there a more idiomatic approach? You could use a single `file(STRINGS)` call and then use `foreach()` with the `IN LISTS` mode to iterate through them. Then use `if(MATCHES)` to extract the individual parts with the `CMAKE_MATCH_` variables. > Also, I'm a bit concerned that they are polluting scope by leaking out > the GIFMAJ/GIFMIN/GIFREL working variables... how would I improve this? Prefix the working variables with _GIF_ and then unset() them at the end. If the version is now expected to work please also update `Tests/CMakeOnly/AllFindModules/CMakeLists.txt` to check that a version number is extracted. In a CMake build tree run `ctest -R CMakeOnly.AllFindModules -V` to run the test. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
[cmake-developers] [PATCH] Improve FindGIF version detection (fix for issue #16196)
A fix for https://gitlab.kitware.com/cmake/cmake/issues/16196 This is my first attempt at doing anything with CMake, so I'd appreciate any feedback on my patch! In particular, the pairs of file()/string() commands seem a bit convoluted for extracting strings out of the header file - is there a more idiomatic approach? Also, I'm a bit concerned that they are polluting scope by leaking out the GIFMAJ/GIFMIN/GIFREL working variables... how would I improve this? --- Modules/FindGIF.cmake | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/Modules/FindGIF.cmake b/Modules/FindGIF.cmake index 7bbb8cf..283a299 100644 --- a/Modules/FindGIF.cmake +++ b/Modules/FindGIF.cmake @@ -61,7 +61,8 @@ set(GIF_LIBRARIES ${GIF_LIBRARY}) # to be always " Version 2.0, " in versions 3.x of giflib. # In version 4 the member UserData was added to GifFileType, so we check for this # one. -# http://giflib.sourcearchive.com/documentation/4.1.4/files.html +# Versions after 4.1.6 define GIFLIB_MAJOR, GIFLIB_MINOR, and GIFLIB_RELEASE +# see http://giflib.sourceforge.net/gif_lib.html#compatibility if(GIF_INCLUDE_DIR) include(${CMAKE_CURRENT_LIST_DIR}/CMakePushCheckState.cmake) include(${CMAKE_CURRENT_LIST_DIR}/CheckStructHasMember.cmake) @@ -71,7 +72,22 @@ if(GIF_INCLUDE_DIR) set(CMAKE_REQUIRED_INCLUDES "${GIF_INCLUDE_DIR}") CHECK_STRUCT_HAS_MEMBER(GifFileType UserData gif_lib.h GIF_GifFileType_UserData ) if(GIF_GifFileType_UserData) -set(GIF_VERSION 4) +# OK. So we're version 4 or higher. Check for specific version defines +file(STRINGS ${GIF_INCLUDE_DIR}/gif_lib.h GIFMAJ REGEX "^[ \t]*#define[ \t]+GIFLIB_MAJOR") +string(REGEX REPLACE "^.*GIFLIB_MAJOR ([0-9]+).*$" "\\1" GIFMAJ ${GIFMAJ}) +# extract minor version +file(STRINGS ${GIF_INCLUDE_DIR}/gif_lib.h GIFMIN REGEX "^[ \t]*#define[ \t]+GIFLIB_MINOR") +string(REGEX REPLACE "^.*GIFLIB_MINOR ([0-9]+).*$" "\\1" GIFMIN ${GIFMIN}) +# point release +file(STRINGS ${GIF_INCLUDE_DIR}/gif_lib.h GIFREL REGEX "^[ \t]*#define[ \t]+GIFLIB_RELEASE") +string(REGEX REPLACE "^.*GIFLIB_RELEASE ([0-9]+).*$" "\\1" GIFREL ${GIFREL}) +if(GIFMAJ) + # yay - got exact version info + set(GIF_VERSION ${GIFMAJ}.${GIFMIN}.${GIFREL}) +else(GIFMAJ) + # couldn't find the defines - assume version 4 + set(GIF_VERSION 4) +endif(GIFMAJ) endif() CMAKE_POP_CHECK_STATE() endif() -- 2.7.4 -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers