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 pre-Python 3.8 C code #124

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.

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]>
@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?

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