PyCode_New API change breaks backwards compatibility policy

[ncoghlan]

BPO	37221
Nosy	@nascheme, @ncoghlan, @scoder, @vstinner, @encukou, @ambv, @serhiy-storchaka, @jdemeyer, @pablogsal, @miss-islington
PRs	bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper #13959 [3.8] bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959) #14505 bpo-37221: PyCode_New() didn't change in Python 3.8 #23595 [3.8] bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595) #23599 [3.9] bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595) #23600

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-07-07.09:10:14.565>
created_at = <Date 2019-06-10.23:24:27.388>
labels = ['build', '3.8', 'release-blocker']
title = 'PyCode_New API change breaks backwards compatibility policy'
updated_at = <Date 2020-12-01.15:55:03.350>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2020-12-01.15:55:03.350>
actor = 'miss-islington'
assignee = 'none'
closed = True
closed_date = <Date 2019-07-07.09:10:14.565>
closer = 'scoder'
components = []
creation = <Date 2019-06-10.23:24:27.388>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 37221
keywords = ['patch', '3.8regression']
message_count = 35.0
messages = ['345153', '345154', '345168', '345172', '345196', '345229', '345230', '345271', '345272', '345279', '345281', '345283', '345284', '345305', '345311', '345312', '345313', '345318', '345334', '345338', '346224', '346389', '346405', '346427', '346993', '346996', '346997', '347004', '347054', '347414', '347415', '347468', '382261', '382264', '382267']
nosy_count = 10.0
nosy_names = ['nascheme', 'ncoghlan', 'scoder', 'vstinner', 'petr.viktorin', 'lukasz.langa', 'serhiy.storchaka', 'jdemeyer', 'pablogsal', 'miss-islington']
pr_nums = ['13959', '14505', '23595', '23599', '23600']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue37221'
versions = ['Python 3.8']

[ncoghlan]

The Porting section of the What's New guide is for changes where the old behaviour was at best arguably correct, but it's still possible someone was relying on it behaving exactly the way it used to.

It isn't for us to say "We broke all extensions that use this existing public C API by adding a new parameter to its signature".

For 3.8b2, the function with the extra parameter should be renamed to PyCode_NewEx, and a PyCode_New compatibility wrapper added that calls it with the extra parameter set to zero.

[vstinner]

PyCode_New() and types.CodeType constructor are actively discussed:

• https://mail.python.org/archives/list/python-dev@python.org/thread/VXDPH2TUAHNPT5K6HBUIV6VASBCKKY2K/
• bpo-36896
• bpo-36886

[pablogsal]

Thanks, Nick for opening this issue. If everyone agrees this is the best path forward I can make a PR. Take into account that doing such rename will break again the projects that have adapted already (primarily only Cython). I would like to give some context around the change itself.

• Last time the function was changed in commit 4f72a78 (for adding keyword-only parameters) a wrapper was not created or a new compatibility layer was not created. This happened as well in the past when the function slowly growed from 3 parameters in 1990 to 16 parameters currently. If we decide to provide the wrapper, this would be the first time we do this for changes in this function.

• In the past, backwards incompatible changes that came from syntax changes were reported in the What's New. For example, in Python3.6 we added "async and await names are now reserved keywords. Code using these names as identifiers will now raise a SyntaxError" in the porting session and one could argue that that broke more code than this change. I am not saying this to justify breaking changes, just to give more context of somehow similar situations on how we report changes that we knew that were backwards incompatible due to new features.

• A quick (and I acknowledge that is partial and incomplete) search in GitHub about usages of PyCode_New excluding interpreter files to avoid forks and the like, points mainly at Cython (and Pyrex forks). Cython already adapted to this change, so changing it back will break Cython and all other possible extensions that already adapted after the last alpha.

• In the email thread https://mail.python.org/archives/list/python-dev@python.org/thread/VXDPH2TUAHNPT5K6HBUIV6VASBCKKY2K/, Stefan Behnel (who is also a Cython maintainer) said in relation to adding PyCode_NewEx:

It's not a commonly used function, and it's easy for C code to adapt. I
don't think it's worth adding a new function to the C-API here, compared to
just changing the signature. Very few users would benefit, at the cost of
added complexity.

[pablogsal]

I have created PR13959 in case we decide to go with the PyCode_NewEx path.

[serhiy-storchaka]

Seems it was changed last time many years ago and was stable in Python 3.

[jdemeyer]

Take into account that doing such rename will break again the projects that have adapted already (primarily only Cython).

+1. You already broke backwards compatibility once in beta1, no need to do it again in beta2.

[pablogsal]

You already broke backwards compatibility once in beta1

There is one other thing to consider, that is projects with pinned dependencies will have to upgrade to the last Cython and update their generated sources if we don't do the renaming. I don't know how often this happens (for example, Python3.7 changed the result of PyUnicode_AsUTF8AndSize() and PyUnicode_AsUTF8() to type const char * rather of char * and that is backward incompatible if you compile with a C++ compiler or with -Werror) but certainly is worth to consider.

Also, take into account that beta period is precisely the moment to fix these kinds of problems (although I apologize for any inconvenience this may have caused).

[vstinner]

I would prefer to revert PyCode_New() API (to Python 3.7 and older API), and add a *new* function for positional-only arguments.

I have created PR13959 in case we decide to go with the PyCode_NewEx path.

I dislike "Ex" suffix. What will be next name? Ex2? NewEx?

I prefer "With" naming.

I suggest: PyCode_NewWithPosArgs(). IMHO it's way more explicit when you opt-in for this new function ;-)

--

PyCode_New() was broken 8 times in the history of Python :-)
https://bugs.python.org/issue37032#msg343377

It seems like more people are unhappy with this backward incompatible change, because Python popularity is still growing. It's a good sign of the health of time :-)

--

+1. You already broke backwards compatibility once in beta1, no need to do it again in beta2.

It's not this easy.

This issue mostly impact projects using Cython. For practical reasons, projects include C files generated by Cython (to avoid Cython dependency to install the project). Cython 0.29.8 is the first version supporting Python 3.8 (new PyCode_New API) was only released two weeks ago.

Right now, I guess that almost all, if not all, tarballs on PyPI on projects using Cython still include C code only compatible with Python 3.7 (old PyCode_New API).

There are 183k projets on PyPI. I would prefer to not have to have to manually regenerate the C code they include to support the new PyCode_New() API.

We are still at beta stage. The role of beta releases is to detect backward incompatible changes like that.

I'm in favor of reverting PyCode_New() API and add a new function for the very few people who care about building manually a code object with positional-only arguments.

It's just a practical move to not break projects on PyPI.

Please, synchronize with Cython to make sure that we can get a Cython release soon after beta2 with will emit code working on all Python version. Maybe the workaround for Python 3.8 alpha1 .. 3.8 beta1 can be simply removed from Cython?

[vstinner]

It seems like very few pepople are testing Python 3.8 so far. I'm making this assumption because many projects using Cython cannot be installed on Python 3.8 and I didn't see much complain so far.

My Python Maintenance team at Red Hat is working on fixing all the stuff for Python 3.8, and we are the most exposed by such backward incompatible change. FYI we modified a few Fedora packages to explicitly run Cython as part of the build to ensure that C files are always regenerated with Cython. So if PyCode_New() API change again in beta2 but Cython takes that into account, Fedora Rawhide will be unaffected.

Note: we are not sure yet if Fedora 31 will be able to switch to Python 3.8 by default, the build of many package is still failing, for various reasons (sadly, this change is far from being the only reason!).

More info:
https://twitter.com/VictorStinner/status/1138067403341012997

[ncoghlan]

The key problem isn't Cython itself, the problem is that Cython generated libraries can't be rebuilt for 3.8 without regenerating their C files.

I'd be fine with PyCode_NewWithPosArgs (Victor's right that a descriptive naming convention handles future changes better than Ex)

[pablogsal]

I updated the PR and Victor reviewed it. Nick, could you review it as well?

[nascheme]

I suggest we change PyCode_New() back in the next beta, despite that change breaking Cython again. We could work with them so that they have a new release with a "PY_VERSION_HEX > 0x030800b1" test in it. Try to get that Cython version released before 3.8b2 hits. The window of a Cython version that doesn't support CPython >= 3.8b1 can be made pretty small. It would be nice to find these issues in alpha releases but finding them in beta is still early enough to fix it.

Having to regenerate all of the Cython emitted code embedded in different packages is painful. I experienced having to update numpy so that I could test my software with 3.8b1. Avoiding that is why we should change PyCode_New back. Introducing a new API like PyCode_NewWithPosArgs() is going to work better.

If we do have to make a similar incompatible change in the future, we should try hard to make it so there is an overlapping period of compatibility. It's bad to have an API change from one release to the next without giving time for projects like Cython time to catch up.