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

Handle API compatibility for CPython bpo-37221 #3009

Merged
merged 2 commits into from
Jun 28, 2019
Merged

Conversation

ncoghlan
Copy link
Contributor

@ncoghlan ncoghlan commented Jun 21, 2019

CPython bpo-37221 reverts the backwards incompatible
change to the PyCode_New API raised in #2938, replacing it with a new
PyCode_NewWithPosOnlyArgs function.

Checking for the new symbol definition allows Cython to
detect any version where bpo-37221 has been resolved,
even if the version number hasn't been updated for the
next beta release yet.

CPython bpo-37221 reverts that backwards incompatible
change to the PyCode_New API, replacing it with a new
PyCode_NewWithPosOnlyArgs function.

Checking for the new symbol definition allows Cython to
detect any version where bpo-37221 has been resolved,
even if the version number hasn't been updated for the
next beta release yet.
@pablogsal
Copy link
Contributor

pablogsal commented Jun 21, 2019

Does Cython's support for positional-only arguments need to use the new PyCode_NewWithPosOnlyArgs for 3.8? If that is the case this fix will not work. Instead, it would need to besoming like:

#if PY_VERSION_HEX < 0x030800A4
  #define __Pyx_PyCode_New(a, p, k, l, s, f, code, c, n, v, fv, cell, fn, name, fline, lnos) 
          PyCode_New(a, k, l, s, f, code, c, n, v, fv, cell, fn, name, fline, lnos)
#else
#if defined(PyCode_NewWithPosOnlyArgs)
    #define __Pyx_PyCode_New(a, p, k, l, s, f, code, c, n, v, fv, cell, fn, name, fline, lnos)
          PyCode_NewWithPosOnlyArgs(a, p, k, l, s, f, code, c, n, v, fv, cell, fn, name, fline, lnos)	          
#else
...

@jdemeyer
Copy link
Contributor

In https://bugs.python.org/issue37221, PyCode_NewWithPosOnlyArgs is not defined as macro but as ordinary function, so this won't work.

@ncoghlan
Copy link
Contributor Author

Ah, you're right - I forgot the preprocessor could only see preprocessor symbols.

So bumping the release serial a few days early still makes the most sense.

@ncoghlan ncoghlan closed this Jun 24, 2019
@ncoghlan ncoghlan reopened this Jun 24, 2019
The preprocessor can't check for compile time symbols
@ncoghlan
Copy link
Contributor Author

Addressed both @pablogsal and @jdemeyer's comments:

  • new check is against the version string, since the preprocessor can see that, but can't see compile time symbols
  • new definition uses the new API, to match the semantics of the 3.8.0a4 and 3.8.0b1 releases

@jdemeyer
Copy link
Contributor

Looks good on first sight. But this is only meaningful when CPython 3.8b2 is released, right (i.e. next week).

@ncoghlan
Copy link
Contributor Author

@jdemeyer Either then, or when the PR for bpo-37221 is merged (if we decide to bump the serial number before the actual release so Cython can continue testing against master)

@ncoghlan
Copy link
Contributor Author

Release manager verdict is that we're going to bump the version as part of the release process as usual, but we'll also wait until Friday to merge the PR that reverts the API change.

So Cython is free to merge this PR whenever you're ready to do so - it will then start doing the right thing as soon as the release is done.

@scoder scoder added this to the 0.29.11 milestone Jun 28, 2019
@scoder scoder merged commit 9b6a02f into cython:master Jun 28, 2019
@scoder
Copy link
Contributor

scoder commented Jun 28, 2019

Thanks, Nick!

scoder pushed a commit that referenced this pull request Jun 28, 2019
CPython bpo-37221 reverts that backwards incompatible
change to the PyCode_New API, replacing it with a new
PyCode_NewWithPosOnlyArgs function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants