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

Remove support for SIP < 5.0.0 #60291

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented Jan 27, 2025

Description

Currently there are a lot of workaround for various old versions of SIP 4.x in QGIS, from passing different CLI flags to using only C++14 instead of C++17 in header files. This PR removes support for SIP 4.x (or any version before 5.0) and removes all these workarounds.

As far as I know, SIP 5.0 or later is shipped with all supported versions of major desktop Linux distributions, so this should be a way to get rid of now-unneeded code without much pain.

I haven't found any policy for which versions of dependencies are supported in QGIS, so I'm going with "whatever non-EOL distros ship". If something else has been the deciding factor up until now, please let me know.

@github-actions github-actions bot added this to the 3.42.0 milestone Jan 27, 2025
@dvdkon dvdkon marked this pull request as draft January 27, 2025 18:47
@dvdkon dvdkon force-pushed the remove-sip-4 branch 2 times, most recently from 82b1b26 to e612d2f Compare January 27, 2025 19:28
@dvdkon dvdkon marked this pull request as ready for review January 27, 2025 19:28
@dvdkon
Copy link
Contributor Author

dvdkon commented Jan 27, 2025

I've had to make some changes to the CI, since some of the platforms didn't have SIP 5/6 installed.

Debian still packages SIP 4 alongside SIP 6, and so does Ubuntu. The CI Dockerfile only installs python3-sip (so SIP 4), despite the current build docs telling users to also install sip-tools (only one is necessary AFAIK, we can drop python3-sip entirely).

The macOS CI has some weird pre-packaged dependency archives. I don't know how they are generated, but presumably someone just needs to include a newer version of SIP in them.

@nyalldawson
Copy link
Collaborator

I'm -1 to this. I still use sip 4 to build qt5, because it's orders of magnitude faster than sip 6.

@dvdkon
Copy link
Contributor Author

dvdkon commented Jan 27, 2025

I'm -1 to this. I still use sip 4 to build qt5, because it's orders of magnitude faster than sip 6.

I'm very much opposed to maintaining support for an old, unmaintained version of software just because it makes build times faster. It has bugs and warts that we need to work around, blocks usage of C++17 in header files, and since SIP 6 is a near-complete rewrite, it's like having two complex and temperamental build tools to worry about.

Sure, the SIP build times are long, but let's instead work to add support for incremental builds to sip-build. That way we'll get the benefit of faster builds even on Qt6.

@nyalldawson
Copy link
Collaborator

It's not just that they are marginally longer, it's that they are workflow-breaking longer 😱. Sip 6 takes upwards of 5 minutes for me for EVERY build, whereas sip 4 is a couple of seconds. QGIS development goes from being enjoyable to a completely painful experience.

I filed a ticket with the sip developer about this and was informed that restoring performance would be a focus of sip 7. I suggest we wait till then (or have some other way of mitigating the speed issue) till proceeding with this.

Copy link

github-actions bot commented Jan 27, 2025

🪟 Windows builds

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

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 3e99e0d)

@nirvn
Copy link
Contributor

nirvn commented Jan 29, 2025

For what it's worth, I also stick to sip 4 to avoid a much slowed down development pace.

@dvdkon
Copy link
Contributor Author

dvdkon commented Jan 29, 2025

I asked around, and at least the original author doesn't plan updating the current macOS dependency packages any time soon, so this will likely only become possible to merge after #60039 and the current Qt5 macOS build is removed.

So we have plenty of time to speed up the SIP6 build until then. Right now, we include everything into one giant file, splitting that up shouldn't be too hard.

@nyalldawson
Copy link
Collaborator

@dvdkon

So we have plenty of time to speed up the SIP6 build until then.

Great! Here's the original thread: https://www.riverbankcomputing.com/pipermail/pyqt/2024-January/045679.html, which includes some profiling of the slow sip 6 build times.

In the meantime, can we close this PR and then re-open when the mac builds are ready for a bump + the speed regression is fixed?

@dvdkon dvdkon marked this pull request as draft February 6, 2025 08:42
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.

3 participants