D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R240:258d6e412435: Bindings: Support using sys paths for 
python install directory (authored by bruns).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15070?vs=42697=44354

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

AFFECTED FILES
  find-modules/FindPythonModuleGeneration.cmake

To: bruns, #frameworks
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  If there are no further comments, I will push this on sunday.

REPOSITORY
  R240 Extra CMake Modules

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

To: bruns, #frameworks
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> FindPythonModuleGeneration.cmake:455
>  install(DIRECTORY 
> ${CMAKE_BINARY_DIR}/py${pyversion}/${GPB_PYTHONNAMESPACE}
> -DESTINATION 
> lib/python${pyversion${pyversion}_maj_min}/site-packages)
>  install(FILES ${sip_files} 
> "${CMAKE_CURRENT_BINARY_DIR}/sip/${GPB_PYTHONNAMESPACE}/${GPB_MODULENAME}/${GPB_MODULENAME}mod.sip"

Hardcoded install directory - keeping this for backwards compatibility!

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  In D15070#347945 , @cgiboudeaux 
wrote:
  
  > In D15070#347944 , @bruns wrote:
  >
  > > So, after another week, no reason has been given not to accept this.
  > >
  > > 1. It fixes broken behavior on several platforms
  > > 2. It does not break current setups
  > > 3. It is consistent with other config variables
  >
  >
  > That's not true, you're refusing to fix the issues. Why should we invest 
time reviewing your changes, exactly?
  
  
  I answered this inline
  
  > - The wrong hardcoded lib/ destination wasn't fixed
  
  If I change this, it breaks the backwards compatibility. It currently is 
broken, and you have to opt-in in the current fixed behaviour.
  
  > - The empty 'if' is still there
  
  And I answered why it is there. Your proposals how to "fix" this leads to 
inconsistent behavior. Changing this in a way which keeps consistent behaviour 
makes the code less readable (either more nesting or longer conditions in the 
if statement). Policy is not there to be followed blindly.

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

D15070: Bindings: Support using sys paths for python install directory

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D15070#347944 , @bruns wrote:
  
  > So, after another week, no reason has been given not to accept this.
  >
  > 1. It fixes broken behavior on several platforms
  > 2. It does not break current setups
  > 3. It is consistent with other config variables
  
  
  That's not true, you're refusing to fix the issues. Why should we invest time 
reviewing your changes, exactly?
  
  - The wrong hardcoded lib/ destination wasn't fixed
  - The empty 'if' is still there

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  So, after another week, no reason has been given not to accept this.
  
  1. It fixes broken behavior on several platforms
  2. It does not break current setups
  3. It is consistent with other config variables

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

D15070: Bindings: Support using sys paths for python install directory

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D15070#344900 , @bruns wrote:
  
  > In D15070#344884 , @cgiboudeaux 
wrote:
  >
  > > In 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

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  In D15070#344884 , @cgiboudeaux 
wrote:
  
  > In 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.

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

D15070: Bindings: Support using sys paths for python install directory

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In 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.

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  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?

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns marked 9 inline comments as done.

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns updated this revision to Diff 42697.
bruns marked 2 inline comments as done.
bruns added a comment.


  Add documentation, remove leftover GPB_PYTHON${pyversion}_SITEARCH

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15070?vs=42631=42697

BRANCH
  python3_support

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

AFFECTED FILES
  find-modules/FindPythonModuleGeneration.cmake

To: bruns, #frameworks
Cc: cgiboudeaux, bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns marked 2 inline comments as done.
bruns added inline comments.

INLINE COMMENTS

> cgiboudeaux wrote in FindPythonModuleGeneration.cmake:39
> KDE_INSTALL_USE_PYTHON${version}_SYS_PATHS shall be added to the doc

Yes, will do.

> cgiboudeaux wrote in FindPythonModuleGeneration.cmake:206-207
> This "if" is not needed if nothing happens.

Structural comment - if KDE_INSTALL_PYTHON${pyversion}DIR is set you can skip 
reading the whole block

> cgiboudeaux wrote in FindPythonModuleGeneration.cmake:209
> This variable is not defined anywhere, this if can be removed.

Yes, leftover ...

> cgiboudeaux wrote in FindPythonModuleGeneration.cmake:216
> elseif(NOT DEFINED KDE_INSTALL_PYTHON${pyversion}DIR)

Depends on which variable you want to win if both (K_I_PYTHONx_DIR and 
K_I_USE_PYTHONx_SYS_PATH) are defined

> cgiboudeaux wrote in FindPythonModuleGeneration.cmake:217
> "lib" is hardcoded. it shouldn't.
> the commit log also mentions the patch uses dist-packages on Debian and its 
> forks. This is not the case here.

Keeping broken behaviour for backwards compatibility, see line 445, 455 in the 
original version.
If not installing below the python prefix, its an arbitrary path anyway.

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

D15070: Bindings: Support using sys paths for python install directory

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> FindPythonModuleGeneration.cmake:39
>  #
>  
>  
> #=

KDE_INSTALL_USE_PYTHON${version}_SYS_PATHS shall be added to the doc

> FindPythonModuleGeneration.cmake:206-207
> +  if(KDE_INSTALL_PYTHON${pyversion}DIR)
> + # Use dir from command line
> +
> +  elseif(KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS)

This "if" is not needed if nothing happens.

> FindPythonModuleGeneration.cmake:208
> +
> +  elseif(KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS)
> +if (NOT GPB_PYTHON${pyversion}_SITEARCH)

if(KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS)

if the option is set, KDE_INSTALL_PYTHON${pyversion}DIR can be safely ignored

> FindPythonModuleGeneration.cmake:209
> +  elseif(KDE_INSTALL_USE_PYTHON${pyversion}_SYS_PATHS)
> +if (NOT GPB_PYTHON${pyversion}_SITEARCH)
> +  execute_process (

This variable is not defined anywhere, this if can be removed.

> FindPythonModuleGeneration.cmake:216
> +
> +  else()
> +set(KDE_INSTALL_PYTHON${pyversion}DIR 
> lib/python${pyversion${pyversion}_maj_min}/site-packages)

elseif(NOT DEFINED KDE_INSTALL_PYTHON${pyversion}DIR)

> FindPythonModuleGeneration.cmake:217
> +  else()
> +set(KDE_INSTALL_PYTHON${pyversion}DIR 
> lib/python${pyversion${pyversion}_maj_min}/site-packages)
> +  endif()

"lib" is hardcoded. it shouldn't.
the commit log also mentions the patch uses dist-packages on Debian and its 
forks. This is not the case here.

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns updated this revision to Diff 42631.
bruns retitled this revision from "Bindings: Query the install directory from 
python" to "Bindings: Support using sys paths for python install directory".
bruns edited the summary of this revision.
bruns edited the test plan for this revision.
bruns added a comment.


  Update summary/test plan

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15070?vs=42629=42631

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

AFFECTED FILES
  find-modules/FindPythonModuleGeneration.cmake

To: bruns, #frameworks
Cc: cgiboudeaux, bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns