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

Add sip5/sip6 sip-build support #45128

Merged
merged 1 commit into from Sep 23, 2021
Merged

Conversation

manisandro
Copy link
Member

No description provided.

cmake/FindSIP.py Outdated Show resolved Hide resolved
@lbartoletti
Copy link
Member

Thanks @manisandro

Let us have time to test on our different systems.

@manisandro
Copy link
Member Author

Yes please, testing welcome

@lbartoletti
Copy link
Member

@manisandro On FreeBSD, and maybe other unix system, it doesn't build unless the WS_X11 tag is set. I'll send you a patch.

But after the fix, I get this sip error:

auto_generated/raster/qgsrasterinterface.sip:498:13: error: no member named 'sipProtect_initHistogram' in 'QgsRasterInterface'
    sipCpp->sipProtect_initHistogram( *a0, a1, a2, minimum, maximum, *a5, a6, a7 );
    ~~~~~~  ^
1 error generated.

tested with sip 5.5.0 and PyQt 5.15.4

@landryb
Copy link
Contributor

landryb commented Sep 20, 2021

@manisandro On FreeBSD, and maybe other unix system, it doesn't build unless the WS_X11 tag is set. I'll send you a patch.

But after the fix, I get this sip error:

auto_generated/raster/qgsrasterinterface.sip:498:13: error: no member named 'sipProtect_initHistogram' in 'QgsRasterInterface'
    sipCpp->sipProtect_initHistogram( *a0, a1, a2, minimum, maximum, *a5, a6, a7 );
    ~~~~~~  ^
1 error generated.

tested with sip 5.5.0 and PyQt 5.15.4

same thing when testing the PR on OpenBSD (same versions of sip & pyqt), backported on top of 3.20.3. Note that it seems i dont need the WS_X11 thing on OpenBSD.

auto_generated/raster/qgsrasterinterface.sip:498:13: error: no member named 'sipProtect_initHistogram' in 'QgsRasterInterface'                                                                                                                                                    
    sipCpp->sipProtect_initHistogram( *a0, a1, a2, minimum, maximum, *a5, a6, a7 );                                                                                                                                                                                                   ~~~~~~  ^                                                                                                                                                                                                                                                                     1 error generated.  

thanks @manisandro for your work, much appreciated !

@manisandro
Copy link
Member Author

Regarding initHistogram: Can you try playing with the code here [1] and see whether tweaking the #if in some way fixes the issue?

[1]

#if defined(SIP_PROTECTED_IS_PUBLIC) or SIP_VERSION >= 0x060000

@landryb
Copy link
Contributor

landryb commented Sep 20, 2021

Regarding initHistogram: Can you try playing with the code here [1] and see whether tweaking the #if in some way fixes the issue?

[1]

#if defined(SIP_PROTECTED_IS_PUBLIC) or SIP_VERSION >= 0x060000

you mean, should that codepath be used with sip 5.5 when using sip-build ?

@manisandro
Copy link
Member Author

Yes, try SIP_VERSION >= 0x050000

@landryb
Copy link
Contributor

landryb commented Sep 20, 2021

Yes, try SIP_VERSION >= 0x050000

my build isnt finished yet, but with SIP_VERSION >= 0x050500 in build-amd64/python/core/auto_generated/raster/qgsrasterinterface.sip it seems all the python stuff has been built so far.. will confirm in a few. That might mean the #if test should be on using sip-build rather than checking the version ?

edit build succeeded. will test runtime now.

@landryb
Copy link
Contributor

landryb commented Sep 20, 2021

afaict, qgis 3.20.3 built from this PR and the SIP_VERSION check relaxed starts fine on OpenBSD. yay!

@manisandro
Copy link
Member Author

Thanks, commit updated. Checking for sip-build rather than SIP_VERSION should be identical, as sip-build will always be used with SIP 5+ with this PR.

@lbartoletti
Copy link
Member

Yes, try SIP_VERSION >= 0x050000

tested & confirmed.

Copy link
Member

@lbartoletti lbartoletti left a comment

Choose a reason for hiding this comment

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

ok FreeBSD@

cc @rhurlin

@rhurlin
Copy link
Contributor

rhurlin commented Sep 21, 2021

ok FreeBSD@

cc @rhurlin

Hi @lbartoletti,
The patches apply and work fine here on my FreeBSD box with recent QGIS devel (c2243a7).

Thanks for notification and congrats for nomination :)

@nyalldawson
Copy link
Collaborator

As with all changes of this nature we're never going to identify any issues upfront. Let's merge and see what build systems break...!

@nyalldawson nyalldawson merged commit 6fd34ca into qgis:master Sep 23, 2021
@nyalldawson
Copy link
Collaborator

@manisandro @lbartoletti @jef-n
This has broken PyQGIS in fundamental ways -- specifically the new sip-build based method does NOT correct set the old "-g" flag to indicate that methods should release the GIL by default.

That's required by QGIS to avoid deadlocks in threaded code and code which raises exceptions, such as Processing. It's the underlying cause behind #45766 and #45685

We need to fix this ASAP, I cannot overstate how bad this regression is.

@nyalldawson
Copy link
Collaborator

Seems like the solution is to set the flag in some pyproject.toml file -- see https://www.riverbankcomputing.com/static/Docs/sip/pyproject_toml.html . But like usual, the sip docs give no clear indication on how to do this or on how pyproject.toml can be used during the build. 😠

@manisandro
Copy link
Member Author

#45829 should address this...

@stoecker
Copy link

stoecker commented Nov 4, 2021

Will the SIP6 support be backported to 3.22? We have lot's of trouble in openSUSE packaging because of missing SIP6 support...

@manisandro
Copy link
Member Author

It is in master and 3.22

@bnavigator
Copy link

bnavigator commented Nov 4, 2021

Please also add #45829 to 3.22

Sorry for the noise. It's in there. Not familiar enough with your release branch setup.
c47fc8a

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

8 participants