Please ensure that wherever it is installed to ends up within CMAKE_INSTALL_PREFIX otherwise this will end up failing on the CI system.

The CI system takes care of setting PYTHONPATH accordingly to ensure resources are found.

In D15070#315030, @bcooksley wrote:

Please ensure that wherever it is installed to ends up within CMAKE_INSTALL_PREFIX otherwise this will end up failing on the CI system.

The CI system takes care of setting PYTHONPATH accordingly to ensure resources are found.

This code essentially asks python where its PATHes point to, so as long as the the installation is consistent it should work as expected.

@bcooksley Of course I can not guarantee it does not break CI, but as long as it follows generic python standards and rules (i.e. properly setup distutils) it should work.

On the other hand, I can guarantee it currently is broken for a number of distributions.

All we do is set PYTHONPATH, to ensure that the directory we install additional things to is searched by Python.

It is imperative however that things are not installed outside of CMAKE_INSTALL_PREFIX otherwise they will be missed by the capture process, and will not be available to future builds that depend on that project.

From my reading of the code this will not be the case, as the value from get_python_lib() is not rebased from the general python install prefix (sys.prefix) to the install prefix given here.
Note that this code will also break installation as a normal user as it will always try to install into the system prefix which won't work (unless you disable it, or run it as root)

If you can change PYTHONPATH for execution, why can't you specify GPB_PYTHONx_SITEARCH?

I would expect if I do not specify a path manually, it should be installed in the default location. This btw. does honor PREFIX/EXEC_PREFIX, see https://docs.python.org/3/distutils/apiref.html#module-distutils.sysconfig
Currently the system default location is not used, and it is not even possible to force the correct location in general (mixture of variables and hardcoded parts).

The default location is also correctly determined if you use e.g. virtualenv.

The canonical path for python user installations is ~/.local/lib/python<major>.<minor>/site-packages/ which would imply a CMAKE_INSTALL_PREFIX of ~/.local/. See also PEP370, https://www.python.org/dev/peps/pep-0370/.

We could, however that would not help people installing it as a regular user, rather than as root.
If I run this right now, as an unprivileged user, I get the following path returned:

>>> sysconfig.get_python_lib( plat_specific=True,standard_lib=False )
'/usr/lib/python2.7/dist-packages'

This will break installation for those doing development installations.

If instead the code were to take CMAKE_INSTALL_PREFIX into account (which it should) then we get a valid result:

>>> sysconfig.get_python_lib( plat_specific=True,standard_lib=False, prefix="/some/path" )

Note: i'm aware of ~/.local/ and virtualenv's, however those not working on Python bindings may just prefer to set PYTHONPATH much like they already set PATH - especially if they have multiple installations they switch between (different Frameworks versions for instance).
'/some/path/lib/python2.7/site-packages'

Note that it's up to the user to set PYTHONPATH properly if they're installing outside the usual directories searched by Python.

In D15070#318493, @bcooksley wrote:

We could, however that would not help people installing it as a regular user, rather than as root.
If I run this right now, as an unprivileged user, I get the following path returned:

>>> sysconfig.get_python_lib( plat_specific=True,standard_lib=False )
 '/usr/lib/python2.7/dist-packages'

cmake -DGPB_PYTHON2_SITEARCH=/foo/bar/ ...
cmake -DGPB_PYTHON2_SITEARCH=~/.local/lib/python2.7/site-packages/ ...
cmake -DGPB_PYTHON2_SITEARCH=~/kf5-python-test1234/ ...

This will break installation for those doing development installations.

If instead the code were to take CMAKE_INSTALL_PREFIX into account (which it should) then we get a valid result:

>>> sysconfig.get_python_lib( plat_specific=True,standard_lib=False, prefix="/some/path" )

Prefix is part of the python installation. If you local python installation is in /usr/local/lib, prefix is that, same if you use e.g. /opt/mypython/ for your python installation.

Note: i'm aware of ~/.local/ and virtualenv's, however those not working on Python bindings may just prefer to set PYTHONPATH much like they already set PATH - especially if they have multiple installations they switch between (different Frameworks versions for instance).
'/some/path/lib/python2.7/site-packages'`

if your python is installed below /some/path, prefix should be /some/path already.

Note that it's up to the user to set PYTHONPATH properly if they're installing outside the usual directories searched by Python.

And why isn't it up to the user to set GPB_PYTHON2_SITEARCH when installing out of the usual directories?

In D15070#318496, @bruns wrote:

And why isn't it up to the user to set GPB_PYTHON2_SITEARCH when installing out of the usual directories?

Because we don't install things out of CMAKE_PREFIX_PATH unless asked explicitly. The logic has to be reversed...and the name changed: GPB_*_SITEARCH is fine as a internal name but noone will remember it when running CMake.

Suggestion:
Do what the KDE_INSTALL_USE_QT_SYS_PATHS option does (OFF by default): If the option is set, the files are installed in the system dirs.

Thanks to arc for throwing my commit message away ...

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?

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.

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.

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

In D15070#345218, @cgiboudeaux wrote:

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

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.

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

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

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.

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