Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Qt6][Sip] Correctly retrieve PyQt module directory #57218

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

troopa81
Copy link
Contributor

On Debian testing Python_SiteSearch is in /usr/local/lib/python3.11 while PyQt6 has been installed in /usr/lib/python3/dist-packages. This PR propose a more robust way to retrieve module directory

@github-actions github-actions bot added this to the 3.38.0 milestone Apr 22, 2024
@m-kuhn
Copy link
Member

m-kuhn commented Apr 22, 2024

Would it be possible to have this relative to CMAKE_INSTALL_PREFIX? IIRC we have an absolute path for py install dir while we have relative paths for everything else (which is problematic on windows with drive letters). Can also be done in a separate PR though, just hijacking to collect opinions ;-)
Should this also be applied in other places? Cmp https://github.com/kadas-albireo/kadas-albireo2/blob/main/vcpkg/ports/qgis/bindings-install.patch

@troopa81
Copy link
Contributor Author

Would it be possible to have this relative to CMAKE_INSTALL_PREFIX?

Regarding this particular modification, I would say no. This is a system path targeting an already installed dependency, not shipped with/by QGIS.

IIRC we have an absolute path for py install dir while we have relative paths for everything else (which is problematic on windows with drive letters). Can also be done in a separate PR though, just hijacking to collect opinions ;-)

I think I remember the issue you describe, and yes we should rely on relative path when dealing with py install dir

Should this also be applied in other places? Cmp https://github.com/kadas-albireo/kadas-albireo2/blob/main/vcpkg/ports/qgis/bindings-install.patch

Maybe. On Windows I imagine that the situation is more simple than on Debian and there is only one Python install dir so it's less needed.

Unfortunately I'm not (yet) skilled enough to make and test the vcpkg modification. I failed to go till the end of the build process last time we try 😬

@m-kuhn
Copy link
Member

m-kuhn commented Apr 23, 2024

Would it be possible to have this relative to CMAKE_INSTALL_PREFIX?

Regarding this particular modification, I would say no. This is a system path targeting an already installed dependency, not shipped with/by QGIS.

Makes sense

IIRC we have an absolute path for py install dir while we have relative paths for everything else (which is problematic on windows with drive letters). Can also be done in a separate PR though, just hijacking to collect opinions ;-)

I think I remember the issue you describe, and yes we should rely on relative path when dealing with py install dir

Thanks for confirming👍

Should this also be applied in other places? Cmp https://github.com/kadas-albireo/kadas-albireo2/blob/main/vcpkg/ports/qgis/bindings-install.patch

Maybe. On Windows I imagine that the situation is more simple than on Debian and there is only one Python install dir so it's less needed.

If you do DESTDIR=C:/my_installation make install on windows it will install into C:/my_installation/C:/where/my/python/is/installed, which works as expected on systems without drive letters where the contents of /my_installation can be relocated to system paths later on.

Unfortunately I'm not (yet) skilled enough to make and test the vcpkg modification. I failed to go till the end of the build process last time we try 😬

This is a simple patch that can be applied to building QGIS unrelated to vcpkg

@troopa81
Copy link
Contributor Author

This is a simple patch that can be applied to building QGIS unrelated to vcpkg

Sorry, I think I get your point now.

You mean modify SIPMacros.cmake to install QGIS in the same Python_SIteSearch that PyQt? Because in the actual state, QGIS python would be installed in one of the valid Python_SiteSearch. So, it should work (not tested though).

But If you do think it's better, I can make the modification

@m-kuhn
Copy link
Member

m-kuhn commented Apr 23, 2024

I think this should be merged as is 👍 (it's about where to find the existing installation and not where to install the results to).

For the future, I'd like to change the behavior and find a solution for the install/prefix path.
For reference there:

  • we have something installed to [system_prefix]/bin, [system_prefix]/lib, ... where system_prefix typically is /usr or / or C:\somewhere\vcpkg_installed or C:\OSGEO4W
  • we are building and installing qgis into an isolated folder somewhere else and only later on move it also into
    system_prefix. So we install with DESTDIR=/home/builder/package now DESTDIR is prepended to all install paths, which works well if they are relative to [system_prefix] but is problematic if it is an absolute path into system prefix.

@troopa81
Copy link
Contributor Author

I think this should be merged as is 👍 (it's about where to find the existing installation and not where to install the results to).

OK, I also added the modification for PyQt5, to stay consistent

For the future, I'd like to change the behavior and find a solution for the install/prefix path.

I don't have a complete picture on how QGIS is working when installing but, we should install everything with relative path! this way, it would always be relative to CMAKE_INSTALL_PREFIX, this one being either a system_prefix (default to /usr/local on linux for instance) or a given DESTDIR.

@troopa81
Copy link
Contributor Author

unrelated test failure

@troopa81
Copy link
Contributor Author

unrelated test failure

@m-kuhn Do you mind merging ?

@m-kuhn m-kuhn enabled auto-merge April 25, 2024 14:08
@m-kuhn m-kuhn disabled auto-merge April 25, 2024 14:09
@m-kuhn m-kuhn merged commit 1f00b40 into qgis:master Apr 25, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants