-
Notifications
You must be signed in to change notification settings - Fork 748
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
Python 3.8 #1138
Conversation
The actual problem to fix is this (from https://docs.python.org/3/reference/datamodel.html#creating-the-class-object):
I'll look into it tomorrow. |
f3714a2
to
82b8fdb
Compare
If `PyCell_Set` is not called with a cell it will raise an exception, which is perfectly valid here.
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. |
@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. |
Codecov Report
@@ Coverage Diff @@
## master #1138 +/- ##
=======================================
Coverage 86.66% 86.66%
=======================================
Files 1 1
Lines 300 300
=======================================
Hits 260 260
Misses 40 40
Continue to review full report at Codecov.
|
There was a problem hiding this 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
src/runtime/typemanager.cs
Outdated
// 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__"); | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM, I found the code to be replica of https://github.com/python/cpython/blob/145bf269df3530176f6ebeab1324890ef7070bf8/Objects/typeobject.c#L2844
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 viatype_new
(== PyType_Type.tp_new
) but instead generate it manually. The added code is done at the end oftype_new
and is required from Python 3.8 onwards (see command below).