-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 special handling of Pyston #4211
Conversation
Looking at the Cython code I spotted a few areas where it contains special paths for Pyston v1. Pyston v1 only supports python2 and is dead but we did release a new Pyston v2 which is a fork of CPython 3.8. As we don't currently set PYSTON_VERSION in v2 the current code can't trigger and can be removed. I hoped we would not need any special casing for v2 because it should be fully compatible with CPython but cython#4200 may temporarily need one but I think the current code is overkill and if necessary I can add (part of) it back later on.
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.
As I understand it:
- Pyston 1 and Pyston 2 are different code-bases but have some of the same people involved.
- Pyston 1 is "dead" in that it's no longer maintained.
Does that mean that nobody is still trying to use Pyston 1? (i.e. Cython probably doesn't want to break something that's actively used, even if it's creators would like to move on)
To me if seems reasonable to leave the 0.29.x Cython series as is (support Pyston 1 to whatever extent it does) and make this change in the Cython 3 alpha only?
@@ -107,64 +106,9 @@ | |||
#undef CYTHON_USE_EXC_INFO_STACK | |||
#define CYTHON_USE_EXC_INFO_STACK 0 | |||
|
|||
#elif defined(PYSTON_VERSION) |
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.
I wonder if we should keep this section (or at least something similar).
I think it's unlikely that Cython will need no hacks to work with Pyston (if nothing else, we may need to disable CPython-specific hacks), and therefore it'd be useful to keep the ability to detect/customize it.
For now it may be worth returning these settings to be the same as CPython by default though.
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.
I would need to change it to check for PYSTON_MAJOR_VERSION
or something similar because we don't define PYSTON_VERSION
right now (in Pyston v2).
I would prefer to only add it in the future if it turns out to be necessary but if you prefer to have it in I will change my commit.
Yes you understood it correctly, Kevin (kmod) and me are they two main developers of Pyston v1 and v2.
I'm not familiar with the Cython version number scheme but this does sound good to me (any rough plans when 0.30 will be released?). v2 is not defining With your your da-woods@5876a2e fix we have working pyston v2 support without needing all the additional workarounds. |
Cython 3 is the right time to remove old cruft, so let's get it in. |
Looking at the Cython code I spotted a few areas where it contains special paths for Pyston v1.
Pyston v1 only supports python2 and is dead but we did release a new Pyston v2 which is a fork of CPython 3.8.
As we don't currently set
PYSTON_VERSION
in v2 the current code can't trigger and can be removed.I hoped we would not need any special casing for v2 because it should be fully compatible with CPython but
#4200 may temporarily need one but I think the current
code is overkill and if necessary I can add (part of) it back later on.