- Notifications
You must be signed in to change notification settings - Fork748
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
A few issues still in the works:
|
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
tof557fb5
CompareMac build appears to be blocked ondotnet/roslyn#46772 |
e0c14cf
to478023d
Compareabaa42f
toc0a751b
Comparelostmsu 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.
namespace Python.Runtime.Native | ||
{ | ||
[StructLayout(LayoutKind.Sequential)] | ||
struct StrPtr : 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.
@@ -22,6 +22,7 @@ public PyDict(IntPtr ptr) : base(ptr) | |||
{ | |||
} | |||
internal PyDict(BorrowedReference reference) : 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
toa6cbe20
Compare
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.PythonDLL
or 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
ILibraryLoader
on the first attempt to call any Python C API functions.Marshaling attributes have been replaced with explicit marshaling calls.
PythonDLL
gets its default value in the following order from:PYTHONNET_PYDLL
environment variablenull
when launched from Python (so that DLL is already loaded in the process).PYTHONNET_PYVER
environment variable (e.g. '3.6' -> 'libpython3.6.so')Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG