On Wednesday 27 January 2010 10:46:49 Frank Schaefer wrote:
> The attached patch makes libv4l mandatory on Linux (only).
> Should be reviewed by someone with more CMake-experience ;).
>
> Thanks,
> Frank
It'd be better if you pasted the contents of the patch here or used
ReviewBoard.
I'll paste the contents of the patch here and comment.
> ===================================================================
> --- cmake/modules/FindLibV4L2.cmake (Revision 1080994)
> +++ cmake/modules/FindLibV4L2.cmake (Arbeitskopie)
> @@ -22,11 +22,15 @@
>
> IF (LIBV4L2_INCLUDE_DIR AND LIBV4L2_LIBRARY)
> SET (LIBV4L2_FOUND TRUE)
> - ENDIF( LIBV4L2_INCLUDE_DIR AND LIBV4L2_LIBRARY )
> -ENDIF ( NOT LIBV4L2_FOUND)
> + ENDIF (LIBV4L2_INCLUDE_DIR AND LIBV4L2_LIBRARY)
> +ENDIF (NOT LIBV4L2_FOUND)
>
> -IF( LIBV4L2_FOUND )
> - IF( NOT LIBV4L2_FIND_QUIETLY )
> - MESSAGE( STATUS "Found LIBV4L2: ${LIBV4L2_LIBRARY}")
> - ENDIF( NOT LIBV4L2_FIND_QUIETLY )
> -ENDIF( LIBV4L2_FOUND )
These (and some of the changes below) are cosmetic changes which I'd rather
see in a separate commit.
> +IF (LIBV4L2_FOUND)
> + IF (NOT LIBV4L2_FIND_QUIETLY)
> + MESSAGE (STATUS "Found LIBV4L2: ${LIBV4L2_LIBRARY}")
> + ENDIF (NOT LIBV4L2_FIND_QUIETLY)
> +ELSE (LIBV4L2_FOUND)
> + IF (LibV4L2_FIND_REQUIRED)
> + MESSAGE (FATAL_ERROR "Could NOT find libv4L2 (library and headers are
needed)")
> + ENDIF (LibV4L2_FIND_REQUIRED)
> +ENDIF (LIBV4L2_FOUND)
This module seems to have skipped the review process in the kde-buildsystem
mailing list before being committed and looks buggy.
The checks you've inserted are already done by the
FindPackageHandleStandardArgs module from CMake. I see it's included in the
beginning of this file, but it isn't being used. A good example of how a
FindXXX.cmake file should look like is kdelibs' FindLibXml2.cmake.
Basically, instead of checking for LIBV4L2_FOUND (I'm talking about both if
clauses, the one with NOT and the one without it), you just look for the
header and the library (preferrably using HINTS in FIND_PACKAGE and
FIND_LIBRARY) and then use something like:
FIND_PACKAGE_HANDLE_STANDARD_ARGS(LibV4L2 DEFAULT_MSG LIBV4L2_LIBRARY
LIBV4L2_INCLUDE_DIR)
After that you usually do this as well:
MARK_AS_ADVANCED(LIBV4L2_INCLUDE_DIR LIBV4L2_LIBRARY)
To summarize, I recommend rewriting the file and sending it to the kde-
buildsystem for a decent review.
> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt (Revision 1080994)
> +++ CMakeLists.txt (Arbeitskopie)
> @@ -100,9 +100,15 @@
> macro_bool_to_01(LIBGADU_FOUND HAVE_LIBGADU)
> macro_log_feature(LIBGADU_FOUND "libgadu" "A library providing support for
Gadu-Gadu protocol" "http://toxygen.net/libgadu/"; FALSE "1.8.0" "Required for
Kopete Gadu-Gadu protocol")
>
> -macro_optional_find_package(LibV4L2)
> +if (CMAKE_SYSTEM_NAME MATCHES Linux)
Isn't it better to use STREQUAL? MATCHES is usually used with a regular
expression.
> + FIND_PACKAGE(LibV4L2 REQUIRED)
> +else (CMAKE_SYSTEM_NAME MATCHES Linux)
> + if (NOT WIN32)
> + macro_optional_find_package(LibV4L2)
> + macro_log_feature(LIBV4L2_FOUND "libv4l2" "Collection of
video4linux support libraries" "http://hansdegoede.livejournal.com/3636.html";
FALSE "" "Required for better webcam support")
> + endif (NOT WIN32)
> +endif (CMAKE_SYSTEM_NAME MATCHES Linux)
> macro_bool_to_01(LIBV4L2_FOUND HAVE_LIBV4L2)
> -macro_log_feature(LIBV4L2_FOUND "libv4l2" "Collection of video4linux
support libraries" "http://hansdegoede.livejournal.com/3636.html"; FALSE ""
"Required for better webcam support")
Actually, you should always use macro_log_feature, and set its _required
argument (the FALSE above) accordingly. My suggestion here is to use
find_package() without REQUIRED, and just set _required in macro_log_feature
to TRUE if necessary -- this way, the whole build process will run and will
fail only at the end, when the list of required packages which were not found
will be listed (this is what kdelibs/CMakeLists.txt does for packages such as
Perl).
>
> check_include_files(valgrind/valgrind.h HAVE_VALGRIND_H)
> check_include_files(stdint.h HAVE_STDINT_H)
Cheers,
Raphael
_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel
