D7736: Not-to-be-merged review of Python bindings generator

[Luca Beltrame]

lbeltrame added a comment.


  A small number of reviews (this thing is huge). One question: would it be possible to have the bindings per-framework, rather than a single, long list? This is also what made PyKDE4 unwieldy. IOW, each Framework should ship their (optional) bindings. 
  We can put the tooling in ECM so that everything is in place for all the Frameworks. (This is how the current approach works):

INLINE COMMENTS

> CMakeLists.txt:49
> +find_package(Clang REQUIRED)
> +find_package(LibClang REQUIRED)
> +

Don't use `find_package(FOO REQUIRED)`. Use `find_package(FOO)` then `set_package_properties(FOO TYPE REQUIRED...`. There are many examples in KDE git you can use. You will need to include FeatureSummary for this to work properly.

Why do I say this? Because now CMake will fail *immediately* at the first error. Otherwise, it will collect and print all missing packages after configuration. It is much easier on the packagers.

> CMakeLists.txt:53
> +    message(SEND_ERROR "Clang++ not found")
> +elseif(SIP_VERSION VERSION_LESS 4.19.3)
> +    message(SEND_ERROR "SIP version \"${SIP_VERSION}\" too old")

Any specific reason for this SIP version?

> FindClang.cmake:52
> +
> +find_program(Clang_EXECUTABLE NAMES clang-3.9)
> +

This won't work on all distros. Can you either:

a. Take inspiration from KDevelop's FindClang (which works already nicely)
b. Use upstream Clang module to find it and its version?

The same comment goes for FindLibClang.

> HOWTO:12
> +
> +Ubuntu/Debian "sudo apt install"
> +--------------------------------

Before distro-specific instructions, put a list of all the dependencies with their upstream names. This ensures that packagers of non Ubuntu/Debian distros can also look them up.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D7736

To: shaheed, lbeltrame
Cc: #frameworks, #build_system
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20170908/d21969cb/attachment.html>