D15070: Bindings: Support using sys paths for python install directory
Stefan BrĂ¼ns
noreply at phabricator.kde.org
Thu Oct 18 11:47:31 BST 2018
bruns added a comment.
In D15070#345218 <https://phabricator.kde.org/D15070#345218>, @cgiboudeaux wrote:
> In D15070#344900 <https://phabricator.kde.org/D15070#344900>, @bruns wrote:
>
> > In D15070#344884 <https://phabricator.kde.org/D15070#344884>, @cgiboudeaux wrote:
> >
> > > In D15070#344871 <https://phabricator.kde.org/D15070#344871>, @bruns wrote:
> > >
> > > > As all the raised concerns have been dealed with, can we give this a try while the next KF release is still somewhat in the future?
> > >
> > >
> > > No, the empty if must be removed. Code that does nothing is useless.
> >
> >
> > Its not empty, there is a comment inside. Of course I can add a `set(KDE_INSTALL_PYTHON${pyversion}DIR ${KDE_INSTALL_PYTHON${pyversion}DIR})` if you insist ...
> >
> > And if you restructure it you either end up with a lengthy if condition - `if (NOT KDE_INSTALL_PYTHON${pyversion}DIR AND KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS)` - or another nesting level. Both are significantly harder to read.
>
>
> If KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS is true, KDE_INSTALL_PYTHON${pyversion}DIR can be ignored. This was in the review
You recommended to follow KDE_INSTALL_USE_QT_SYS_PATHS, so I did. KDE_INSTALL_USE_QT_SYS_PATHS changes the default values, but still allows to override the individual paths.
After your proposed change, KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS wins over KDE_INSTALL_PYTHON${pyversion}DIR, which is inconsistent behaviour.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D15070
To: bruns, #frameworks
Cc: cgiboudeaux, bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20181018/ef526d8b/attachment.html>
More information about the Kde-buildsystem
mailing list