-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Python 2 removal #3688
Python 2 removal #3688
Conversation
#error BYE_BYE_GOLDEN_SNAKE
Let's always use PY_VERSION_HEX, better to be consistent. Let's make this < 3.6, no need to drop 2.7 and < 3.6 in separate steps. |
#3683 prereq? |
I'll make the PY_VERSION_HEX change, but I very strongly disagree dropping 3.5 in the same PR: lumping such clearly distinct steps together is a recipe for getting into trouble. Earlier in my career I got burned many times when I did that. It's a risk, and I don't see an upside that could justify taking that risk. Additionally, if there are questions later, backtracking will be much easier if we keep the version drops separate. |
The 1 previous CI failure (https://github.com/pybind/pybind11/runs/5080406438?check_suite_focus=true) was a well-known flake. |
The force push was just for |
Please do not do things like this by hand. Pyupgrade does this for you. Update pyupgrade in pre-commit first, then only do manual cleanup as needed. |
include/pybind11/detail/common.h
Outdated
@@ -213,6 +213,9 @@ | |||
|
|||
|
|||
#include <Python.h> | |||
#if PY_VERSION_HEX < 0x03000000 | |||
#error "PYTHON 2 IS NO LONGER SUPPORTED. pybind11 v2.9.1 was the last to support Python 2." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#error "PYTHON 2 IS NO LONGER SUPPORTED. pybind11 v2.9.1 was the last to support Python 2." | |
#error "PYTHON < 3.6 IS NO LONGER SUPPORTED. pybind11 v2.9 was the last to support Python 2.7-3.5." |
sys.stdout = original | ||
|
||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so ugly. If we had used sys.version_info < (x, y)
, then pyupgrade would automatically clean up this sort of stuff for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing is true for all our custom PY2 markers. If we used the sys.version_info idiom, then pyupgrade does it automatically. Any MyPy understands it. Etc.
Nice to see all this cleanup! Mind if I use pyupgrade here? |
You must bump python_requires. It's the single most important thing to bump, and it must be done first, otherwise you'd break Python 2 (and 3.5, let's drop both in one shot), instead of just dropping it. I'll apply a few of these things now, you can remove them if they are not helpful or clash. :) I haven't started on 3.5 yet, to make sure you were okay with that. |
Feel free to take it from here. I was hoping that you could help with the things I'm not familiar with (setup tools, cmake), but maybe it's better if you take the lead on everything. As I explained before, I strongly feel it's a mistake to handle something as major as the Python 2 removal, together with a minor version drop for the next major version, but maybe we'll get lucky and there are no accidents, and the care I wanted to give to making the git history easy to navigate is not as important as I think. I'll review this PR when you're done. Please let me know. |
IMO, and from doing this drop over a dozen times, we should be doing one action: changing the minimum version supported to 3.6. Doing it in stages is loading more work on ourselves; I'm already changing things to >3.5 when I'm going too have to go back and do it again with 3.6 - If I can remember / can re-grep it. Anyway, done for now, will revisit later. |
I looked through your 4 commits, those all look great to me. My vote would be to merge this as soon as possible, so that we don't run into merge conflicts with other development and maintenance work. The only open question is, what do we want to do about the Centos7 block? I'll trigger the CI in another (scratch) PR to see if that's still broken. I see Python 3.5 is still there, which I still think is best. I'm guessing the Python 3.5 version drop will only be a few lines in comparison. Unless we have a strong and compelling reason for mixing those in now, it will be more useful to have those changes visible as a separate step in the git history. To have the same obviousness for the C++ code, the disciplined & most auditable approach would be:
I'm guessing again the C++ Python 3.5 removal will be much smaller, but even if both parts of the 3.5 removal are small, keeping them as separate PRs will be a clear assurance (especially for people looking later) that the C++ code wasn't changed when the tests were manipulated in the first step, and test changes to go with the C++ changes, if any, can be easily identified and audited, because they are separate. |
The force push was again required b/o |
.pre-commit-config.yaml
Outdated
@@ -29,13 +29,13 @@ repos: | |||
- id: mixed-line-ending | |||
- id: requirements-txt-fixer | |||
- id: trailing-whitespace | |||
- id: fix-encoding-pragma | |||
exclude: ^noxfile.py$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude: ^noxfile.py$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ This is a hook specific include that should be removed along with the offending hook.
I went ahead and bumped pre-commit properly. |
We should merge the clang format PR first, so we have a common point to make a branch for backports. That’s the point. If we already started dropping then we don’t have that. |
Nice! I added a mention to the PR description. |
Backports for Python 2.7? — I hadn't thought of that. I firmly agree though, even a weak reason is enough to prefer merging the clang-format PR before this one, at least if we move quickly, as I hope, so that this PR doesn't suffer from bit rot symptoms. |
The force push was purely for |
You can see what changed in force-pushes by clicking "force pushed" above. You don't have to explain every force push as long as they are for rebasing, squashing, or adding things missing in an original comment. ;) |
6c0caa6
to
cf5a439
Compare
We have to rewrite this anyway, since it should say "pybind11 2.9 was the last version to support Python <3.6". |
The Python 2 drop is much more special, I think for a couple years this will be helpful: pybind11 2.9 was the last version to support Python 2, and <3.6 While mathematically speaking that's redundant, I believe for practical purposes that's exactly what people will want to know. |
revert: intree
Description
This PR removes Python 2 support completely, and Python 2-isms almost completely.
Please see the commit history of this PR for the cleanup steps. The cleanup is assumed to be comprehensive.
The Python 2 removal unblocks updating black, which is included in this PR (03c8bb1).
Suggested changelog entry: