Review Request 110953: Stylistic changes to FindBoostPython.cmake and framework

David Narváez david.narvaez at computer.org
Thu Jul 11 05:10:54 UTC 2013



> On June 11, 2013, 4:58 p.m., David Narváez wrote:
> > KigConfigureChecks.cmake, line 12
> > <http://git.reviewboard.kde.org/r/110953/diff/1/?file=149558#file149558line12>
> >
> >     What would you say about moving this and the macro_log_feature call below to CMakeLists.txt? I believe it is more visible there.
> 
> Vadim Zhukov wrote:
>     Personally I don't see any point in having KigConfigureChecks.cmake file at all. But this is probably for another patchset, to not complicate all current Kig reviews. Okay to post-pone this task?
> 
> David Narváez wrote:
>     I agree, so I think moving the Boost Python stuff outside KigConfigureChecks.cmake takes us a step towards removing that file completely so that's my reason to wonder if we could do that in this patch.

As 4.11 branches are now created, I'm retaking this (and the other) patch in order to commit them to master (as we were already pushing one rather large refactor to FindBoostPython.cmake in 4.11 and I didn't want to mix these up after the first betas). After applying it I have two more comments:

- My boost python detection failed for a reason that has nothing to do with this patch, but my configure messages ended like this:

[...]
-- Found PkgConfig: /usr/bin/pkg-config (found version "0.28") 
-- Looking for C++ include boost/shared_ptr.hpp
-- Looking for C++ include boost/shared_ptr.hpp - found

-----------------------------------------------------------------------------
-- The following OPTIONAL packages could NOT be located on your system.
-- Consider installing them to enable more features from this software.
-----------------------------------------------------------------------------
   * Boost.Python (1.31 or higher)  <http://www.boost.org/>
     Kig can optionally use Boost.Python for Python scripting

-----------------------------------------------------------------------------

-- Configuring done
[...]

a regular user would be clueless of what went wrong, can we improve feedback in the sections after finding shared_ptr.hpp?

- Also, you see the message above reads 1.31 or higher, but FindBoostPython.cmake has a line that reads find_package(Boost 1.33 COMPONENTS python), so we need to pick one of the two, the oldest one.


- David


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


On June 11, 2013, 3:08 p.m., Vadim Zhukov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110953/
> -----------------------------------------------------------
> 
> (Updated June 11, 2013, 3:08 p.m.)
> 
> 
> Review request for KDE Edu and David Narváez.
> 
> 
> Description
> -------
> 
> Polish FindBoostPython.cmake from CMake's point of view:
>   
>  * Use BoostPython_ prefix instead of BOOST_PYTHON_
>    (BOOSTPYTHON_ is also acceptable but is unreadable);
> 
>  * Remove extra call to find_package(BoostPython);
> 
>  * Replace macro_optional_find_package() with simple find_package(),
>    CMake has CMAKE_DISABLE_FIND_PACKAGE_Foo feature for a while.
> 
> This is a first patch of a bigger set, containing only minor fixes.
> 
> 
> This addresses bug 320807.
>     http://bugs.kde.org/show_bug.cgi?id=320807
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 3c3da29 
>   KigConfigureChecks.cmake fed38b3 
>   cmake/modules/FindBoostPython.cmake d6c5a34 
> 
> Diff: http://git.reviewboard.kde.org/r/110953/diff/
> 
> 
> Testing
> -------
> 
> OpenBSD-CURRENT, CMake 2.8.11, KDE 4.10.4, Boost 1.53, Python 3.3 and 2.7.
> 
> 
> Thanks,
> 
> Vadim Zhukov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20130711/2a4fa1f2/attachment.html>


More information about the kde-edu mailing list