Skip to content

Remove pre-Python 3.8 C code #124

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rtobar
Copy link
Contributor

@rtobar rtobar commented Sep 5, 2024

This is a bit of a big one, but it's mostly removal of dead code anyway, so hopefully it's not too controversial, and well received.

While doing some further code inspection today to re-tackle #86 I realised that there's still a fair bit of code in the C extension, and in the C extension generator, for Python < 3.8. Since 3.8 is the minimum supported version, it makes sense to remove that now-dead code in favour of a less complex codebase.

In order to make this a bit more digestible I split this work in two commits: one focuses on removing support for Python <3, and another for <3.8. Many of these changes are big #if blocks that could easily be removed. In a few places there were macros that were defined differently depending on the Python version, that would have now been left with a single definition. In these cases I removed the macro altogether in favour of using the direct Python C API or idiom as required (e.g., all the PyText_* macros are now PyUnicode_* calls, etc).

Both commits are (I think) correctly self-contained, at least locally they individually both compile, install and test correctly. So in case something breaks it should be easy to bisect back.

@arigo
Copy link
Contributor

arigo commented Sep 5, 2024

See #113 for my personal stance on this issue. @nitzmahone If wants to merge this anyway, go ahead.

@rtobar
Copy link
Contributor Author

rtobar commented Sep 5, 2024

@arigo thanks for that input. I also maintain two libraries for which I kept 2.7 support until not too long ago, but finally decided to drop it, with the rationale being that 2.7 users still can use the old versions without lacking much functionality anyway. Kind-of-supporting 2.7 (or anything other than what setup.py states) will add a maintenance cost that officially you shouldn't need to pay.

I'm not sure if you're aware of this, but have a look at https://pypistats.org/packages/cffi. Seeing those numbers was also something that prompted me to drop support for <1% of users, who again can still download earlier versions of my library. Removing hundreds of lines of C code and macros I think pays the price.

My opinion only of course, if you decide you want to keep the code that's fine, you know your audience and needs better, I won't be pushing back.

@nitzmahone
Copy link
Member

Yeah, especially with the very deep inspection and review of all the code that's going to be necessary for proper support of free-threaded Python builds, I'm all for minimizing compatibility code in the active development branches for Python versions that aren't supported/tested.

My experience has repeatedly taught me: if it's not getting tested, it's probably broken...

The old branches of course aren't going anywhere, so if someone really needs to make something work on a long-dead Python version, they can always install an older release or backport fixes to a private fork. We've had several firedrills in recent memory caused by "dead but still called" code breaking suddenly, so I'm quite happy to be more aggressive about cleaning that stuff up for 1.18 and beyond. Thanks!

@nitzmahone nitzmahone added this to the 1.18 milestone Sep 6, 2024
@rtobar
Copy link
Contributor Author

rtobar commented Sep 6, 2024

Thanks @nitzmahone, that sounds pretty good to me :)

I have been building some further changes on top of these in the hopes that they'd be merged. Based on your message I'll continue under that assumption.

Would you be happy for a similar effort in the python codebase?

@rtobar
Copy link
Contributor Author

rtobar commented Oct 18, 2024

Gentle ping here to see if the intention of doing this clean-up is still there, thanks! I've just also rebased on top of the latest main branch.

Most of this is removing code for PY_VERSION_MINOR < 3, removing the
guards for PY_VERSION_MAJOR >= 3, and removing all unnecessary macros
that would now have a single definition (e.g., PyText_Check ->
PyUnicode_Check) in favour of using the direct Python C API for clarity.
Only in minor circumstances some small logic needed to be changed.

Signed-off-by: Rodrigo Tobar <[email protected]>
Similarly to the previous commit, the bulk of the work is removing code
for PY_VERSION_HEX < 0x03080000, removing #if guards around
PY_VERSION_HEX >= 0x03080000, and removing unnecessary macro
definitions.

Signed-off-by: Rodrigo Tobar <[email protected]>
@rtobar
Copy link
Contributor Author

rtobar commented Dec 29, 2024

Another gentle ping after a few months of inactivity

@rtobar rtobar force-pushed the remove-pre38-c-code branch from 8aa897d to db6983c Compare December 29, 2024 11:54
@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

I think it would be fine to remove pre-python3 from the src/c code and the vengine_cpy.py code for one PR. There is still a need to somewhat support Python-2 for PyPy-2, which still needs CFFI, even though PyPy vendors CFFI into its codebase. Once we do a first cleanup, we can see what is left.

@mattip
Copy link
Contributor

mattip commented Jun 25, 2025

Rereading #113, I think we should release 1.18 as the last version to still support Python2, and merge something like this PR together with the free-threading code for a new version (1.19? 2.0?). Then people who still wish to use CFFI on Python2 can pin it to 1.18

@nitzmahone
Copy link
Member

I think we should release 1.18 as the last version to still support Python2, and merge something like this PR together with the free-threading code for a new version (1.19? 2.0?).

Is there actually anything unreleased or pending merge to main that's apropos for the older pypy branches? If not, the 1.17 branch is still there (and worst case, we could always backport whatever for a 1.17.2 in the future). IIUC the desire for a published pre-release with free-threaded support is the only time-sensitive item right now, so it'd be nice to avoid the churn of doing a 1.18 with nearly no changes and 1.19 with FT support concurrently.

@nitzmahone
Copy link
Member

nitzmahone commented Jun 25, 2025

Another thought: for the pypy branches that would be orphaned by removal of old code from main- could their "current CFFI" checks be retargeted to the release-1.17 branch (as the last branch that would contain updates they could sync)? Then main going forward (which gets branched off for 1.18, 2.0, or whatever, don't care what we call it) can have old code ripped out at our leisure, and anything critical in the future that might be needed by the older pypy branches can just be backported to the 1.17 branch (with or without tagging/release, TBD).

@arigo
Copy link
Contributor

arigo commented Jun 25, 2025

My 2 cents: it would be nice if removing-Python-2-support and free-threading-support would both be released into cffi 2.0. It makes it clearer IMHO that it's the first couple of major changes to cffi that occurred for a long time. PyPy2 and users of old CPythons could be sticking with the latest cffi 1.x version. Future updates to cffi 2.0 would only need to be ported to PyPy3, which might remove a few hurdles. The jump to version 2.0 makes it clear when that change occurred.

@mattip
Copy link
Contributor

mattip commented Jun 26, 2025

it'd be nice to avoid the churn of doing a 1.18

OK, this is reasonable.

could their "current CFFI" checks be retargeted to the release-1.17 branch

Good idea.

It would be nice if removing-Python-2-support and free-threading-support would both be released into cffi 2.0.

We could do 2.0 with free-threaded support and partial Python2 cleanup (i.e. what is needed to make the merge of #178 easier) while declaring it no longer supports Python2. Then we could continue the cleanup in a 2.1. This would allow a faster release of the 2.0 free-threading support.

@rtobar
Copy link
Contributor Author

rtobar commented Jun 27, 2025

I've been reading this these last messages with interest. Please let me know if you want me to rebase these changes on top of the latest main.

For those who haven't seen the detail, this PR has two commits:

  • The first removes C code for python 2
  • The second removes C code for python < 3.8

If we go ahead, do we want to:

  • Keep both commits
  • Declare 3.8 and unsupported, since that python version is discontinued. Maybe it's been removed already from cffi, I haven't checked.
  • Remove C code for python 3.8

@mattip
Copy link
Contributor

mattip commented Jun 28, 2025

I would prefer to separate the two commits. If these statistics can be believed, 3.8 still has about a 10% share on urllib3 downloads on PyPI

@mattip
Copy link
Contributor

mattip commented Jun 28, 2025

… although in the pyproject.toml there is already this which came in PR #129:

requires-python = ">=3.9"

@nitzmahone was that intentional?

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.

4 participants