Skip to content

Python 3.8 #1138

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

Merged
merged 8 commits into from
May 16, 2020
Merged

Python 3.8 #1138

merged 8 commits into from
May 16, 2020

Conversation

filmor
Copy link
Member

@filmor filmor commented May 10, 2020

Adds Python 3.8 to the CI and fixes the remaining issue by assigning __classcell__ the created class if it exists. This is required as for the case of subclassing a C# class in Python, we (currently) don't initialise the type via type_new (== PyType_Type.tp_new) but instead generate it manually. The added code is done at the end of type_new and is required from Python 3.8 onwards (see command below).

@filmor
Copy link
Member Author

filmor commented May 10, 2020

The actual problem to fix is this (from https://docs.python.org/3/reference/datamodel.html#creating-the-class-object):

CPython implementation detail: In CPython 3.6 and later, the __class__ cell is passed to the metaclass as a __classcell__ entry in the class namespace. If present, this must be propagated up to the type.__new__ call in order for the class to be initialised correctly. Failing to do so will result in a RuntimeError in Python 3.8.

I'll look into it tomorrow.

@filmor filmor force-pushed the python38 branch 2 times, most recently from f3714a2 to 82b8fdb Compare May 14, 2020 23:12
@filmor filmor marked this pull request as ready for review May 14, 2020 23:13
@filmor filmor requested a review from lostmsu May 14, 2020 23:13
filmor added 2 commits May 15, 2020 01:28
If `PyCell_Set` is not called with a cell it will raise an exception,
which is perfectly valid here.
@filmor
Copy link
Member Author

filmor commented May 15, 2020

Okay, so one bug left to fix: On Linux with .NET Core and Python 3.8 some of the embedding tests fail, will have a look.

@filmor
Copy link
Member Author

filmor commented May 15, 2020

@lostmsu Tests are now passing for all cases as far as I can see, can you have a look? I'll update the Changelog and then this is good to go.

@filmor filmor linked an issue May 15, 2020 that may be closed by this pull request
@codecov-io
Copy link

codecov-io commented May 15, 2020

Codecov Report

Merging #1138 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1138   +/-   ##
=======================================
  Coverage   86.66%   86.66%           
=======================================
  Files           1        1           
  Lines         300      300           
=======================================
  Hits          260      260           
  Misses         40       40           
Flag Coverage Δ
#setup_linux 65.33% <ø> (ø)
#setup_windows 72.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8f5ab0...ed2b7e8. Read the comment docs.

Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

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

Apart from NITs looks fine. However, we should not remove public members from Runtime in minor release.

- Use `BorrowedReference` where applicable
- Readd `OperatingSystemName` and `MachineName` for now
@filmor filmor requested a review from lostmsu May 16, 2020 11:24
@filmor filmor merged commit 782eff8 into master May 16, 2020
@filmor filmor deleted the python38 branch May 16, 2020 18:21
Comment on lines 283 to 290
// Update the __classcell__ if it exists
IntPtr cell = Runtime.PyDict_GetItemString(cls_dict, "__classcell__");
if (cell != IntPtr.Zero)
{
Runtime.PyCell_Set(cell, py_type);
Runtime.PyDict_DelItemString(cls_dict, "__classcell__");
}

Copy link
Member

Choose a reason for hiding this comment

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

@filmor I can't find a test, that would cover this scenario, and I suspect there might be a problem with reference counting: if __classcell__ is present, its content is a PyType*, which might need to be DecRefed before being replaced with py_type, and py_type might need to be IncRefed before being put into the cell.

Also, I don't understand why __classcell__ is removed later. If it is removed, what is the purpose of updating its value?

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalise Python 3.8 support
3 participants