[Kde-graphics-devel] Review Request 110962: Switch to an external LibRaw

[Vadim Zhukov]

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



cmake/modules/FindLibRaw.cmake
<http://git.reviewboard.kde.org/r/110962/#comment25107>

    Better "the additional compiler definitions..." to avoid confusion.



cmake/modules/FindLibRaw.cmake
<http://git.reviewboard.kde.org/r/110962/#comment25110>

    It's probably better to have own prefix for _version_* variables too, e.g.: "_libraw_version_major_match".



cmake/modules/FindLibRaw.cmake
<http://git.reviewboard.kde.org/r/110962/#comment25112>

    So what if upstream changes:
    
    #define LIBRAW_MAJOR_VERSION 1
    
    to:
    
    enum     LIBRAW_MAJOR_VERSION     1   // my major version
    
    ? Or what if those definitions go to another header file, like libraw_version_1.h, and libraw_version.h becomes a stub?



cmake/modules/FindLibRaw.cmake
<http://git.reviewboard.kde.org/r/110962/#comment25111>

    CMake is all over strings, maybe just omit "_STRING" in "LibRaw_VERSION_STRING"? This is bikeshedding, of course, just to the sake of consistency.



libkdcraw/CMakeLists.txt
<http://git.reviewboard.kde.org/r/110962/#comment25108>

    Why MACRO_OPTIONAL_FIND_PACKAGE()? find_package() in CMake itself have CMAKE_DISABLE_FIND_PACKAGE_Foo feature for a while.



libkdcraw/CMakeLists.txt
<http://git.reviewboard.kde.org/r/110962/#comment25109>

    This looks like fishy. You probably want to find_library() instead, don't you?


- Vadim Zhukov


On June 12, 2013, 1:28 a.m., Pino Toscano wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110962/
> -----------------------------------------------------------
> 
> (Updated June 12, 2013, 1:28 a.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/kde-graphics-devel/attachments/20130612/99b0475f/attachment-0001.html>