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.

Sign up for GitHub

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

Jump to bottom

bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper #13959

Merged
merged 6 commits into from
Jul 1, 2019
Merged

bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper #13959

merged 6 commits into from
Jul 1, 2019

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 11, 2019 •
edited by bedevere-bot
Loading

@pablogsal pablogsal requested a review from tiran as a code owner June 11, 2019 01:48
@pablogsal pablogsal requested review from ncoghlan and vstinner June 11, 2019 01:51
@pablogsal pablogsal requested a review from scoder June 11, 2019 01:56
@vstinner
Copy link
Member

I would prefer to call the new function PyCode_NewWithPosArgs() rather than PyCode_NewEx().

@vstinner
Copy link
Member

Would it make sense to wait to merge this PR just before next beta2, and also synchronize with Cython to make sure that Cython will be compatibl with Python 3.8 beta2, soon after beta2 is released?

@pablogsal
Copy link
Member Author

I renamed the new function to PyCode_NewWithPosOnlyArgs

@pablogsal
Copy link
Member Author

Would it make sense to wait to merge this PR just before next beta2, and also synchronize with Cython to make sure that Cython will be compatible with Python 3.8 beta2, soon after beta2 is released?

We can merge and backport it and it will be released before in beta2, right? I will do the PR for Cython right away. They can chose to merge it when they think is better.

@pablogsal pablogsal changed the title bpo-37221: Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper bpo-37221: Add PyCode_NewWithPosArgs to be used internally and set PyCode_New as a compatibility wrapper Jun 11, 2019
@pablogsal pablogsal changed the title bpo-37221: Add PyCode_NewWithPosArgs to be used internally and set PyCode_New as a compatibility wrapper bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper Jun 11, 2019
vstinner
vstinner reviewed Jun 11, 2019
View reviewed changes
vstinner
vstinner requested changes Jun 11, 2019
View reviewed changes
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner
vstinner approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would prefer a second review. @methane, @serhiy-storchaka, @ncoghlan, @encukou, anyone?

@scoder
Copy link
Contributor

scoder commented Jun 12, 2019

Would it make sense to wait to merge this PR just before next beta2

Yes, please. Otherwise, we will loose a month of CI time during which Cython cannot adapt (because the CPython API version does not change before the release) and during which users cannot properly test with the unreleased CPython master because Cython does not work on it any more.

serhiy-storchaka
serhiy-storchaka reviewed Jun 12, 2019
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a What's New entry.

@serhiy-storchaka
Copy link
Member

Would be nice if Cython release a new version which will use a special case only for Python == 3.8b1, not for >= 3.8b1.

@pablogsal
Copy link
Member Author

@serhiy-storchaka There is already a what's new entry. Do you mean something extra or something different?

@serhiy-storchaka
Copy link
Member

Sorry, I missed it.

ncoghlan
ncoghlan approved these changes Jun 15, 2019
View reviewed changes
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pablogsal

@ncoghlan
Copy link
Contributor

@serhiy-storchaka The issue for Cython is that the 3.8 dev branch is currently still mostly advertising itself as 3.8b1: https://github.com/python/cpython/blob/3.8/Include/patchlevel.h

@ambv Could we do something a little odd here, and change the branch version string to be "3.8b2-" and bump the release serial to "2" before the actual release? That way the change could be merged, and Cython would be able to correctly detect it as being a different build from 3.8b1.

@scoder
Copy link
Contributor

scoder commented Jun 16, 2019 via email

@pablogsal
Copy link
Member Author

I have rebased to fix the merge conflicts.

@ambv
Copy link
Contributor

ambv commented Jun 25, 2019

@ambv Could we do something a little odd here, and change the branch version string to be "3.8b2-" and bump the release serial to "2" before the actual release?

Sorry, missed the notification on this one. I'd rather not, let's just merge this shortly before beta2, say: Friday.

@encukou
Copy link
Member

encukou commented Jun 26, 2019

I read that as an invitation for anyone to hit the green button on Friday. Let us know if you want to do the merge yourself.

@encukou encukou merged commit 4a2edc3 into python:master Jul 1, 2019
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-14505 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 1, 2019
@pablogsal @miss-islington
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and se…
2a657ef 
…t PyCode_New as a compatibility wrapper (pythonGH-13959)

Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper
(cherry picked from commit 4a2edc3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
ambv pushed a commit that referenced this pull request Jul 1, 2019
@miss-islington @pablogsal @ambv
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and se…
cb083f7 
…t PyCode_New as a compatibility wrapper (GH-13959) (#14505)

Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper
(cherry picked from commit 4a2edc3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@tirkarthi tirkarthi mentioned this pull request Jul 1, 2019
Closed
@scoder
Copy link
Contributor

scoder commented Jul 1, 2019

FYI, Cython 0.29.11 has been released with the necessary change for beta-2.

@vstinner
Copy link
Member

vstinner commented Jul 1, 2019

FYI, Cython 0.29.11 has been released with the necessary change for beta-2.

That's great! Thank you.

This was referenced Jul 8, 2019
Closed
Merged
@Harmon758 Harmon758 mentioned this pull request Jul 25, 2019
Closed
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@pablogsal @lisroach
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and se…
91c4234 
…t PyCode_New as a compatibility wrapper (pythonGH-13959)

Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@pablogsal @DinoV
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and se…
c351d6d 
…t PyCode_New as a compatibility wrapper (pythonGH-13959)

Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper
@vstinner vstinner mentioned this pull request Feb 18, 2020
Merged
Ark-kun added a commit to Ark-kun/pipelines that referenced this pull request Feb 19, 2020
@Ark-kun
SDK - Fix SDK on Python 3.8
d161b00 
Fixes the follwoing error: "TypeError: code() takes at least 14 arguments (13 given)".

The cause of the issue is a breaking change in CodeType constructor in Python 3.8.
https://bugs.python.org/issue37221
This should have been fixed by python/cpython#13959 and python/cpython#14505, but the code still fails.
@Ark-kun Ark-kun mentioned this pull request Feb 19, 2020
Merged
k8s-ci-robot pushed a commit to kubeflow/pipelines that referenced this pull request Feb 24, 2020
@Ark-kun
SDK - Fix SDK on Python 3.8 (#3126)
2dee255 
* SDK - Fix SDK on Python 3.8

Fixes the follwoing error: "TypeError: code() takes at least 14 arguments (13 given)".

The cause of the issue is a breaking change in CodeType constructor in Python 3.8.
https://bugs.python.org/issue37221
This should have been fixed by python/cpython#13959 and python/cpython#14505, but the code still fails.

* Simplified the replace call
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@Ark-kun @Jeffwan
SDK - Fix SDK on Python 3.8 (kubeflow#3126)
b05e5cb 
* SDK - Fix SDK on Python 3.8

Fixes the follwoing error: "TypeError: code() takes at least 14 arguments (13 given)".

The cause of the issue is a breaking change in CodeType constructor in Python 3.8.
https://bugs.python.org/issue37221
This should have been fixed by python/cpython#13959 and python/cpython#14505, but the code still fails.

* Simplified the replace call
@pablogsal pablogsal deleted the bpo37221 branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@vstinner vstinner vstinner approved these changes

@scoder scoder scoder approved these changes

@ncoghlan ncoghlan ncoghlan approved these changes

@tiran tiran Awaiting requested review from tiran

@ambv ambv Awaiting requested review from ambv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@pablogsal @vstinner @bedevere-bot @scoder @serhiy-storchaka @ncoghlan @ambv @encukou @miss-islington @the-knights-who-say-ni