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

PyCode_New API change breaks backwards compatibility policy #81402

Closed
ncoghlan opened this issue Jun 10, 2019 · 35 comments
Closed

PyCode_New API change breaks backwards compatibility policy #81402

ncoghlan opened this issue Jun 10, 2019 · 35 comments
Labels
3.8 only security fixes build The build process and cross-build release-blocker

Comments

@ncoghlan
Copy link
Contributor

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
    Copy link
    Contributor Author

    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.

    @ncoghlan ncoghlan added 3.8 only security fixes release-blocker build The build process and cross-build labels Jun 10, 2019
    @vstinner
    Copy link
    Member

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

    @pablogsal
    Copy link
    Member

    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
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

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

    @jdemeyer
    Copy link
    Contributor

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    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
    Copy link
    Member

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

    @nascheme
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    If we do have to make a similar incompatible change in the future, (...)

    IMHO the most important learnt lesson here is that we lack a proper Continuous Integration of the master branch of Python and popular PyPI projects. We should be notified *before* a release.

    @scoder
    Copy link
    Contributor

    scoder commented Jun 12, 2019

    Note that PyCode_New() is not the only change in 3.8 beta1 that breaks Cython generated code. The renaming of "tp_print" to "tp_vectorcall" is equally disruptive, because Cython has (or had) a work-around for CPython (mis-)behaviour that reset the field explicitly to NULL after calling PyType_Ready(), which could set it arbitrarily without good reason.

    So, either revert that field renaming, too, or ignore Cython generated modules for the reasoning about the change in this ticket.

    I'm fine with keeping things as they are now in beta-1, but we could obviously adapt to whatever beta-2 wants to change again.

    @pablogsal
    Copy link
    Member

    Adding Petr, that was involved with the "tp_print" to "tp_vectorcall" renaming.

    @jdemeyer
    Copy link
    Contributor

    Technically, tp_print was replaced by tp_vectorcall_offset.

    But that doesn't answer the question how we should deal with tp_print backwards compatibility. Cython does

    FooType.tp_print = 0;

    With this in mind, simply replacing tp_print by tp_vectorcall_offset is unsafe as it would break types that actually use vectorcall (there aren't many for now, but who knows how this will change in the future).

    It would be safer to replace tp_print by tp_vectorcall since setting that to 0 won't break anything (neither for now, nor when PR 13930 is merged).

    @jdemeyer
    Copy link
    Contributor

    PR 14009 deals with tp_print

    @vstinner
    Copy link
    Member

    Note that PyCode_New() is not the only change in 3.8 beta1 that breaks Cython generated code. The renaming of "tp_print" to "tp_vectorcall" is equally disruptive, because Cython has (or had) a work-around for CPython (mis-)behaviour that reset the field explicitly to NULL after calling PyType_Ready(), which could set it arbitrarily without good reason.

    Can someone please open a separated issue to discuss tp_print backward incompatible change? This issue is about PyCode_New().

    @jdemeyer
    Copy link
    Contributor

    I think that the PyCode_New() compatibility problem and tp_print are sufficiently closely related that they can be discussed together.

    In any case, I agree that it makes little sense to fix just one.

    @vstinner
    Copy link
    Member

    Jeroen Demeyer:

    I think that the PyCode_New() compatibility problem and tp_print are sufficiently closely related that they can be discussed together.

    IMHO these problems are different enough to justify to have a separated issue: I created https://bugs.python.org/issue37250 Please discuss tp_print issue there.

    @ncoghlan
    Copy link
    Contributor Author

    I'm happy with the change in #13959, but we need sign-off from Łukasz as release manager on bumping the Py_VERSION_SERIAL a bit early so Cython can reliably detect that this change has been reverted without having to check for the existence of the PyCode_NewWithPosArgs symbol.

    Alternatively, if my proposal at cython/cython#3009 to check directly for PyCode_NewWithPosOnlyArgs in the Cython module setup code is accepted, then we wouldn't need to bump the version number early, and could merge the PR as soon as the What's New conflict is resolved.

    @ncoghlan
    Copy link
    Contributor Author

    In reviewing cython/cython#3009, Jeroen pointed out that my symbol checking idea wouldn't actually work, since the preprocessor can only see preprocessor definitions, not compiler symbols. If we're going to rely on a preprocessor definition, it may as well be PY_VERSION_HEX, so I've updated the Cython PR accordingly.

    That means we're back to either breaking Cython compatibility with CPython master until the release is cut, or else just bumping the release serial a little early. The duration of either state is now a lot shorter though, since the target release date for 3.8.0b2 is next week.

    @vstinner
    Copy link
    Member

    That means we're back to either breaking Cython compatibility with CPython master until the release is cut

    Yeah, that was my plan: merge PR 13959 "just before" beta2, and try to get a Cython release ASAP.

    The best case would be to get a Cython release with the fix *before* beta2 is released. Would it be possible Stefan?

    @scoder
    Copy link
    Contributor

    scoder commented Jun 24, 2019

    I'm really only waiting for bpo-37250 to be resolved, then I can prepare a new point release of Cython, preferably this week.

    @encukou
    Copy link
    Member

    encukou commented Jul 1, 2019

    New changeset 4a2edc3 by Petr Viktorin (Pablo Galindo) in branch 'master':
    bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959)
    4a2edc3

    @vstinner
    Copy link
    Member

    vstinner commented Jul 1, 2019

    cython/cython#3009 was merged. Is it part of Cython 0.29.11 released yesterday?

    @encukou
    Copy link
    Member

    encukou commented Jul 1, 2019

    cython/cython#3009 was merged.

    (to master, so far)

    Is it part of Cython 0.29.11 released yesterday?

    It should be, see: https://cython.readthedocs.io/en/latest/src/changes.html#id2

    @ambv
    Copy link
    Contributor

    ambv commented Jul 1, 2019

    New changeset cb083f7 by Łukasz Langa (Miss Islington (bot)) in branch '3.8':
    bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959) (bpo-14505)
    cb083f7

    @scoder
    Copy link
    Contributor

    scoder commented Jul 1, 2019

    Is it part of Cython 0.29.11 released yesterday?

    Yes.

    @encukou encukou closed this as completed Jul 5, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Jul 6, 2019

    I reopen the issue. Cython 0.29.11 doesn't work on Python 3.8 beta2. Installation fails with a compiler error:

    Cython-0.29.11/Cython/Plex/Scanners.c:7244:274: error: macro "__Pyx_PyCode_New" requires 16 arguments, but only 15 given

    Cython commit 0d88839168013fd69350d31eaee5514cd2f727b9 is not part of Cython tag 0.29.11.

    Stefan: it seems like a new Cython release is needed.

    @vstinner vstinner reopened this Jul 6, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Jul 6, 2019

    I created cython/cython#3034 "Cython doesn't work on Python 3.8 beta2"

    @scoder
    Copy link
    Contributor

    scoder commented Jul 7, 2019

    No need to keep this bug open on CPython side. The backwards compatibility has been restored (and I'll release Cython 0.29.12 today to resolve the issue on that side.)

    @scoder scoder closed this as completed Jul 7, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Dec 1, 2020

    New changeset 1867b46 by Victor Stinner in branch 'master':
    bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
    1867b46

    @miss-islington
    Copy link
    Contributor

    New changeset d9d63f1 by Miss Islington (bot) in branch '3.8':
    bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
    d9d63f1

    @miss-islington
    Copy link
    Contributor

    New changeset ed46143 by Miss Islington (bot) in branch '3.9':
    bpo-37221: PyCode_New() didn't change in Python 3.8 (GH-23595)
    ed46143

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes build The build process and cross-build release-blocker
    Projects
    None yet
    Development

    No branches or pull requests

    10 participants