On Sunday 31 January 2010 11:28:54 Frank Schaefer wrote:
> Raphael Kubo da Costa schrieb:
> >> +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.
>
> I dont' know, I just extended/modified the existing macro.
> I also didn't know that these changes have to be sent to a special
> mailing-list.
> Looking at the policy-docs again now: this seems to be required only for
> changes in kdelibs/cmake/modules/ !?
>
> http://techbase.kde.org/Policies/CMake_Commit_Policy
Neither the initial file nor the subsequent changes _need_ to be sent to the
kde-buildsystem mailing list, however if someone commits a new FindFoo.cmake
file somewhere without any review from the people who have more knowledge of
CMake, the chances of the file being broken or wrong are higher :)
> > 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.
>
> Could you do that, please ? ;)
I'll try to do that within the next hours.
> As I already mentioned: my
> cmake-knowledge is still limited.
> Maybe you can fix some of the warnings about other library-checks (IIRC
> about "PKGCONFIG(...) beeing depricated"), too.
Well, so is mine :) My time is a bit limited, but we can try to fix these
things little by little.
> >> 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.
>
> The examples (official cmake-doc) I've seen for CMAKE_SYSTEM_NAME were
> using MATCHES.
> What's the bebefit of STREQUAL ?
I guess STREQUAL would be slightly faster since it doesn't involve parsing a
regular expression, but I may be wrong. On Linux and other systems which have
the uname command, CMAKE_SYSTEM_NAME is set to the output of "uname -s"; the
downside of using STREQUAL would be not matching if "uname -s" didn't return
exactly "Linux" on Linux.
> Btw, it would be better to use if (LINUX), but that doesn't exist (only
> WIN32, UNIX, APPLE).
> CMAKE_SYSTEM_NAME would not work for cross-compiling, right ? ;)
I'm not sure, it may be a good idea to ask about this and STREQUAL vs MATCHES
on the kde-buildsystem mailing list.
> >> + 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).
>
I think there's a misunderstanding below: what you said you wanted is exactly
what I said too :)
> No, the behavior should depend on the platform / OS:
> - Windows: no libv4l available
> => do not search for it
> => do not log anything
> - Linux: libv4l is mandatory
> => use FIND_PACKAGE(LibV4L2 REQUIRED)
> => logs the result, critical error if package is missing (before
> build start)
> - others: libv4l is optional
> => use macro_optional_find_package(LibV4L2)
> => use macro_log_feature to log the result in the "option packages"
> section
What I suggested is something like the following:
if (NOT WIN32)
set (LIBV4L2_REQUIRED FALSE)
if (CMAKE_SYSTEM_NAME STREQUAL Linux)
set (LIBV4L2_REQUIRED TRUE)
endif (CMAKE_SYSTEM_NAME STREQUAL Linux)
find_package(LibV4L2)
macro_log_feature(LIBV4L2_FOUND "libv4l2" "Collection of video4linux support
libraries" LIBV4L2_REQUIRED "" "Required for better webcam support")
endif (NOT WIN32)
macro_bool_to_01(LIBV4L2_FOUND HAVE_LIBV4L2)
I haven't tested it, but something along these lines should do what we both
want.
> I prefer getting the error-message about missing packages BEFORE the
> build process starts.
Exactly. By "build process" I meant "cmake stuff before compiling things".
> The suggested changes also keep the code consistent with all the other
> existing kopete library-checks / modules.
Yep. Apart from the KDE modules, all the other checks are for optional
libraries, so we don't have much of a precedent here ;)
Cheers,
Raphael
_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel
