Review Request 110962: Switch to an external LibRaw

[Vadim Zhukov]

> On June 11, 2013, 10: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
> 
> Pino Toscano wrote:
>     @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: Because compile check is more reliable than getting version via parsing libraw_version.h. E.g., what if in the next version of libraw the libraw_version.h will be renamed/merged into some other header file, and libraw_version.h will become no-op? I'm all for using find_package_handle_standard_args() when it makes things easier but not for making things worse.

Anyway, I'm not a committer, I'm just a packager with quiet a few CMake patches for KDE. :) I had other set of patches for libkdcraw, which enabled using both internal and external versions of the libraw, but of course it would be better to just remove internal copy of libraw.


- Vadim


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


On June 11, 2013, 9: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, 9: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/06832f02/attachment.html>