- Notifications
You must be signed in to change notification settings - Fork768
Build single Python.Runtime.dll for all platforms#1365
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lostmsu commentedJan 22, 2021
A few issues still in the works:
|
lostmsu commentedJan 22, 2021
Ideally, we should vend |
filmor commentedJan 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
On Linux,
That is the plan, with |
2cbce6a tof557fb5Comparelostmsu commentedJan 23, 2021
Mac build appears to be blocked ondotnet/roslyn#46772 |
e0c14cf to478023dCompareabaa42f toc0a751bComparelostmsu commentedJan 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
One check is failing still, that's the same one everywhere else. |
Uh oh!
There was an error while loading.Please reload this page.
| namespacePython.Runtime.Native | ||
| { | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| structStrPtr:IDisposable |
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.
MaybeNativeString would be more appropriate?
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.
Naming things :-D
Not sure aboutNativeString. It has connotations to native string type supported on the platform, but this is not it.
It might make sense to have aStrPtr type per encoding for clarity. Then signatures could enforce it. But maybe that would be overengineering.
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.
Do we use any encoding apart from UTF8? I thought allFromString functions nowadays used that.
Indeed, I find exactly one usage withEncoding.ASCII, everything else isUTF8. And the one case that uses ASCII isPyBuffer_SizeFromFormat, which doesn't specify ASCII encoding either, I highly doubt it will break if it's passed UTF8 instead as it will probably just compare bytes directly and bail out on anything that it doesn't understand.
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.
The main thing I find "problematic" is that this object is not a pure pointer, it has ownership over the buffer it points to.
| { | ||
| } | ||
| internalPyDict(BorrowedReferencereference):base(reference){} |
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.
Maybe we should renamePyDict to something likePyDictFacade to make it clear that it doesn't take ownership.
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.
This overload actually does incref internally (which makes sense, since to create a potentially long-livedPyDict object pointing to something we borrowed, we need toIncRef).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| internal static extern IntPtr PyObject_GetAttr(IntPtr pointer, IntPtr name); | ||
| internal static int PyObject_SetAttrString(IntPtr pointer, string name, IntPtr value) | ||
| { | ||
| using var namePtr = new StrPtr(name, Encoding.UTF8); |
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.
Since this pattern is used in a lot of places, we should have a function for this (GetUtf8String or something like that). That could open up potential optimisation paths with caching or preallocated buffers per thread.
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.
At this point it would be a premature optimization. Also, I don't see how we could cache disposablestruct instances easily.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -2185,6 +2276,539 @@ internal static IntPtr GetBuiltins() | |||
| { | |||
| return PyImport_Import(PyIdentifier.builtins); | |||
| } | |||
| private static class Delegates | |||
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.
Please makeRuntime apartial class and move this into a separate file.
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.
I can do this just before pushing. Otherwise it would require me to update the roslyn package, that does automatic conversion of [DllImport] into the pattern withDelegates.
| { | ||
| // only in 3.9+ | ||
| } | ||
| PyBuffer_IsContiguous = (delegate* unmanaged[Cdecl]<ref Py_buffer, char, int>)GetFunctionByName(nameof(PyBuffer_IsContiguous), GetUnmanagedDll(_PythonDll)); |
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.
Did you generate this via a script? That would be a good addition (although I'd hope we wouldn't need too many changes of this class ;)).
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.
The script convertedDllImport declarations. It did not work on the Python source, headers, or docs. Essentially, signatures are identical to what we had exceptstring parameters and a few minor exceptions where I switched to*Reference types.
workaround fordotnet/roslyn#49760
should fix Mac build
675ec5f toa6cbe20Compare
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
This allows building the same Python.Runtime.dll on all platforms and supported Python versions.
To invoke Python code from .NET, one needs to set
Python.Runtime.Runtime.PythonDLLor manually preload DLL before invoking any Python.NET functions or other properties.Implementation details
PInvoke signatures have been replaced with unmanaged functions pointers, that are loaded using
ILibraryLoaderon the first attempt to call any Python C API functions.Marshaling attributes have been replaced with explicit marshaling calls.
PythonDLLgets its default value in the following order from:PYTHONNET_PYDLLenvironment variablenullwhen launched from Python (so that DLL is already loaded in the process).PYTHONNET_PYVERenvironment variable (e.g. '3.6' -> 'libpython3.6.so')Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG