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

fix sid build with sip 6.9.1 #59995

Merged
merged 1 commit into from
Dec 23, 2024
Merged

fix sid build with sip 6.9.1 #59995

merged 1 commit into from
Dec 23, 2024

Conversation

jef-n
Copy link
Member

@jef-n jef-n commented Dec 22, 2024

The build on debian sid is currently broken, because it has sip 6.9.1, hence uses the deprecation messages for sip, which also requires a minimal sip abi of 12.16 / 13.9.

But sip-build defaults to 12.13 / 13.6 from pyqtbuild:

https://github.com/Python-PyQt/PyQt-builder/blob/82162b4966d94054733ff9aa53f4ef4ec63d2222/pyqtbuild/builder.py#L137-L144

although it requires 12.16 / 13.9 for deprecation messages:

https://github.com/Python-SIP/sip/blob/a619d79626cfff6678d194c7dd8d0d948bb644c0/sipbuild/generator/utils.py#L496-L499

The patch adds the required abi-version to the pyproject.tomls.

@github-actions github-actions bot added this to the 3.42.0 milestone Dec 22, 2024
@jef-n jef-n requested review from 3nids and troopa81 December 22, 2024 17:21
cmake/SIPMacros.cmake Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 22, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 30a3cc4)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 30a3cc4)

Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this @jef-n !

It looks good. Where did you get that we have to use major version 13 for Qt 6 and 12 for Qt 5 ? I look into the SIP documentation and find nothing.

@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

LGTM and regarding Qt5 vor Qt6, that is in the pyqt[56]-SIP code

@troopa81
Copy link
Contributor

regarding Qt5 vor Qt6, that is in the pyqt[56]-SIP code

Can you be more specific ?

@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

@troopa81
Copy link
Contributor

OK. Thanks @t0b3

@troopa81 troopa81 closed this Dec 23, 2024
@troopa81 troopa81 reopened this Dec 23, 2024
@jef-n jef-n merged commit 8825f86 into qgis:master Dec 23, 2024
62 of 63 checks passed
@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

@jef-n does it compile on sid with this merged?
which pyqt6-sip version does it provide?

unfortunately here on fedora 41 this commit now produces errors
see https://github.com/qgis/QGIS/actions/runs/12465823531/job/34795836439?pr=59595#step:13:13174
probably due to the combination of pyqt6-sip v13.8.0-1 with sip v6.9.0

how can cmake detect the ABI version instead to react accordingly then?
@philthompson10 your input is very welcome here

cc @troopa81 we still need some improvements to get this going...

@troopa81
Copy link
Contributor

troopa81 commented Dec 23, 2024

@t0b3 IMHO fedora needs to ship PyQt-sip-13.9 along with sip-6.9.0 to be fully compliant. In this case PyQt-sip would support the 13.9 ABI

If this is not possible, we need to detect the sip ABI in our cmake to set our ABI minimum version accordingly and enable or not /Deprecated/ with message. I would prefer to go option the first option than going into complicated stuff but if there is no other way...

@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

@troopa81 can you give more details i.e. compliant to what specifically?
I don't know whether fedora 41 would provide updated pyqt6-sip then ...

OTOH sip ABI detection won't hurt, let's see if maybe Phil could show an easy way forward here

@jef-n
Copy link
Member Author

jef-n commented Dec 23, 2024

@jef-n does it compile on sid with this merged? which pyqt6-sip version does it provide?

sure - but using qt5.

@troopa81
Copy link
Contributor

@t0b3 If I understand it correctly (which is not 100% sure)

You see in PyQt-sip-13.8 ship a sip.h file with a ABI_MINOR_VERSION version 8. QGIS retrieve the SIP module from PyQt SIP module, so even if it's build with sip 6.9.0, your are not garanteed to have the latest SIP minor version (9 actually). So this PyQt sip module supports actually only 13.8 and is not enough to support the option /Deprecated with message which appear in 6.9.0. We enable this option if we have sip 6.9.0, so that's why it fails here.

@jef-n
Copy link
Member Author

jef-n commented Dec 23, 2024

It looks good. Where did you get that we have to use major version 13 for Qt 6 and 12 for Qt 5 ? I look into the SIP documentation and find nothing.

Well, see the linked code - pyqtbuild (which provides the abi_version to sipbuild) reports the minimal version 12.13 / 13.6 no matter what version is actually installed and sip-build requires 12.16 / 13.9 for the deprecation messages. So unless the abi-version is explictly set in pyproject.toml it should always fail - and that's what happening on sid - and as said independently from what version is actually installed (would also be 12.16 / 13.9 on sid).

@jef-n
Copy link
Member Author

jef-n commented Dec 23, 2024

OTOH sip ABI detection won't hurt, let's see if maybe Phil could show an easy way forward here

if pyqtbuild would report the actual sip abi version, it would work too

@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

@t0b3 If I understand it correctly (which is not 100% sure)

You see in PyQt-sip-13.8 ship a sip.h file with a ABI_MINOR_VERSION version 8. QGIS retrieve the SIP module from PyQt SIP module, so even if it's build with sip 6.9.0, your are not garanteed to have the latest SIP minor version (9 actually). So this PyQt sip module supports actually only 13.8 and is not enough to support the option /Deprecated with message which appear in 6.9.0. We enable this option if we have sip 6.9.0, so that's why it fails here.

💯 my understanding

That's why it's not enough to rely on the sip version, it might be foolproof to rely on the ABI version.

@jef-n
Copy link
Member Author

jef-n commented Dec 23, 2024

That's why it's not enough to rely on the sip version, it might be foolproof to rely on the ABI version.

Is pyqtbuild not used on fedora? Because AFAICT that hardwiredly reports 12.13 / 13.6 as the abi_version, and that's what sip-build later uses to check whether deprecation messages can be used. That's why sip-build complains about 12.16 and 13.9 being required although they are installed.

@troopa81
Copy link
Contributor

Is pyqtbuild not used on fedora? Because AFAICT that hardwiredly reports 12.13 / 13.6 as the abi_version, and that's what sip-build later uses to check whether deprecation messages can be used. That's why sip-build complains about 12.16 and 13.9 being required although they are installed.

PyQt-builder default version has been fixed: Python-PyQt/PyQt-builder#25

But your PR should resolve the setting of the minum required ABI version. So the only remaining problem here is that SIP module shipped in PyQt and used by QGIS has not been updated (still 13.8 while we need 1.9).

My understanding is that PyQt-builder/PyQt-sip and SIP minor version must be consistent in fedora packages.

@philthompson10
Copy link

philthompson10 commented Dec 23, 2024 via email

@t0b3
Copy link
Contributor

t0b3 commented Dec 23, 2024

IIUC Phils config of abi-version has the meaning of minimum required abi version.
see also his answer here https://www.riverbankcomputing.com/pipermail/pyqt/2021-May/043919.html

so it might be useful to directly rely on the ABI version

@troopa81
Copy link
Contributor

If a project has a requirement for a minimum ABI then it should specify that in its pyproject.toml.

Yes, but here it depends. At compile time, If it's 13.9 or greater we enable /deprecated with message, else we disable them. So we are able to build on several different platform. but how can we do that ?

include the module ABI as an attribute of the sip module (like SIP_VERSION)

Yes, that would help certainly in such situation. I don't see how we can manage to fix here without this (except by having PyQt-sip and SIP minor version aligned in fedora)

@t0b3
Copy link
Contributor

t0b3 commented Dec 24, 2024

Changes I could make if they might be useful...

  • include the module ABI as an attribute of the sip module (like
    SIP_VERSION)
  • add an --abi-version command line option to the build tools (sip-build
    etc) to override the value in pyproject.toml
  • add a pseudo-timeline for the ABI version (similar to the one for
    SIP).

@philthompson10 these would be great additions, especially the first would help here a lot 🎁 🎄

If it's soon available to query the ABI version, QGIS may rely directly on this information and adapt it's sources accordingly to get the build passing 😃 While this may not catch all the recent cases, this brings an easy and solid fix.

t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 24, 2024
t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 24, 2024
@philthompson10
Copy link

philthompson10 commented Dec 24, 2024 via email

t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 24, 2024
t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 24, 2024
get build running for sip ABI < 13.9 with sip >= 6.9.
addresses qgis#59995 (comment)
t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 24, 2024
get build running for sip ABI < 13.9 with sip >= 6.9.
addresses qgis#59995 (comment)
@philthompson10
Copy link

philthompson10 commented Dec 27, 2024 via email

@t0b3
Copy link
Contributor

t0b3 commented Dec 28, 2024

@jef-n while this PR resolves the build issues through enforcing sip-abi to match sip and ignoring maybe lower PyQt-sip ABI. unfortunately this introduces runtime issues in case of PyQt-sip ABI lower than sip ABI, i.e. PyQt6-sip ABI 13.8 and sip ABI 13.9. This case occurs in fedora 41 and debian sid, it also can be reproduced in gentoo.

import qgis._gui
RuntimeError: the sip module implements ABI v13.0 to v13.8 but qgis._gui module requires ABI v13.9

CC @philthompson10 @troopa81

t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 29, 2024
t0b3 added a commit to t0b3/QGIS that referenced this pull request Dec 31, 2024
@philthompson10
Copy link

philthompson10 commented Jan 2, 2025 via email

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.

6 participants