Review Request 110962: Switch to an external LibRaw

[Pino Toscano]

> On June 11, 2013, 6:03 p.m., Vadim Zhukov wrote:
> > cmake/modules/FindLibRaw.cmake, line 36
> > <http://git.reviewboard.kde.org/r/110962/diff/1/?file=149621#file149621line36>
> >
> >     This check looks ugly. LibRaw itself provides LIBRAW_CHECK_VERSION() macros (in libraw_version.h), which could be used in compile check. This way it should be more future-compatible than parsing header file itself.
> 
> Rolf Eike Beer wrote:
>     I bet that macro is a C macro, not a CMake one. So for finding out in CMake code which version was found this doesn't help.
> 
> Pino Toscano wrote:
>     Yes, I know about the macros in libraw_version.h, but they cannot be used at all in version checks at CMake time. Currently, the version string is just printed, but in the future it might be used to force a minimum version.
> 
> Vadim Zhukov wrote:
>     If you want only to get the libraw's version, then you could use libraw_version() function. Just compile sample code which calls libraw_version() and prints its output, and save output to the variable. As a bonus you'll early check if found libraw could be used at all.
> 
> Rolf Eike Beer wrote:
>     @Pino: then simply unset() the variables, including the LIBRAW_VERSION_CONTENT one.
>     
>     @Vadim: that simply will not work, think about cross compiling.
>     
>     One way to avoid parsing the version header most of the time will be using the version from pkg-config, if present.
> 
> Vadim Zhukov wrote:
>     @Rolf: Yes, you're right about cross-compilation case. From the other side, we basically don't need to retrieve the version - we need to check if version we want is sufficient (if LIBRAW_FIND_VERSION was supplied). In this case we could compile the code which looks like the following:
>     
>     #include <libraw.h>
>     #include <libraw_version.h>
>     #if !LIBRAW_COMPILE_CHECK_VERSION_NOTLESS(${LIBRAW_MAJOR}, ${LIBRAW_MINOR})
>     #error "LibRaw version mismatch"
>     #endif

@Vadim: Why should be a test program needed while find_package_handle_standard_args handle the version check already?

The rest of the discussion is just overly pedantry on a private uninstalled module which is to be used only by libkdcraw.


- Pino


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110962/#review34162
-----------------------------------------------------------


On June 11, 2013, 5:50 p.m., Pino Toscano wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110962/
> -----------------------------------------------------------
> 
> (Updated June 11, 2013, 5:50 p.m.)
> 
> 
> Review request for KDE Graphics, Release Team and Gilles Caulier.
> 
> 
> Description
> -------
> 
> Instead of using an embedded copy of LibRaw, look for an external LibRaw as mandatory dependency with a new CMake module and using its variables.
>     
> Considering some LibRaw versions seem to be underlinked and not linking to OpenMP, link it manually in libkdcraw to overcome such lack.
>     
> Switch back to the MAKE_KDCRAW_LIB define (i.e. the default set by KDE4_ADD_LIBRARY) as the one used to check whether it is being built, as otherwise LIBRAW_BUILDLIB would conflict with LibRaw.
> 
> Once this RR is approved, I will remove the libraw code copy and the CMake modules (FindLCMS2.cmake and FindPthreads.cmake) needed for it.
> 
> 
> This addresses bug 307146.
>     http://bugs.kde.org/show_bug.cgi?id=307146
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt f2f269609feb10947ec3bac10125b379c6c821dd 
>   cmake/modules/FindLibRaw.cmake PRE-CREATION 
>   libkdcraw/CMakeLists.txt cce5d6dba690fb5182638ccd1f10488bbd6ec2ce 
>   libkdcraw/libkdcraw_export.h 1a222a03502a0e068bdba4f03b7ff4961c4a8f2b 
> 
> Diff: http://git.reviewboard.kde.org/r/110962/diff/
> 
> 
> Testing
> -------
> 
> Compiles fine with both LibRaw 0.14.7 and 0.15.1.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/release-team/attachments/20130611/77873de6/attachment-0001.html>