Drop LoadLibrary #880
Drop LoadLibrary #880
Conversation
|
I'll have to look a bit more into this. I thought that this |
|
@filmor @amos402 I made a single Python.Runtime.dll to work with any Python 3.x interpreter in my fork (could have included 2.x, but its lifetime is ending) via extensive use of |
|
@lostmsu What do you mean by "extensive use"? Isn't it enough to load libpython*.so once at startup? Could you point me to that code? |
|
@filmor well, the And also losttech@80aa07f All of that happens once at startup too. But there are many functions. There is probably a performance impact too, due to the use of delegates in place of PInvoke calls. |
|
@lostmsu What's your conclusion on keeping |
src/runtime/runtime.cs
Outdated
| @@ -297,16 +297,20 @@ internal static void Initialize(bool initSigs = false) | |||
| IntPtr dllLocal = IntPtr.Zero; | |||
| var loader = LibraryLoader.Get(OperatingSystem); | |||
There was a problem hiding this comment.
Seems like you should delete these lines too, and line 295 as well since you moved it below.
* Drop C module dependency when getting _PyObject_NextNotImplemented * Exception details for SetNoSiteFlag
Codecov Report
@@ Coverage Diff @@
## master #880 +/- ##
=======================================
Coverage 86.71% 86.71%
=======================================
Files 1 1
Lines 301 301
=======================================
Hits 261 261
10000
Misses 40 40
Continue to review full report at Codecov.
|
|
I removed the dependency of |
src/runtime/runtime.cs
Outdated
| { | ||
| dllLocal = loader.Load(_PythonDll); | ||
| throw new Exception($"Cannot load {_PythonDll}"); |
There was a problem hiding this comment.
Can you throw more informative exception? Win32Exception would automatically read last error on Windows, but I don't know what to do for *nix
There was a problem hiding this comment.
Generally it should throw inside from Load, the nullptr checking just for in case(if the loader implementation didn't throw an exception). I'm not going to insert any platform specific code here.
src/runtime/runtime.cs
Outdated
| private static IntPtr Get_PyObject_NextNotImplemented() | ||
| { | ||
| IntPtr globals = PyDict_New(); | ||
| if (PyDict_SetItemString(globals, "__builtins__", PyEval_GetBuiltins()) != 0) |
There was a problem hiding this comment.
Can PyEval_GetBuiltins() return an error?
There was a problem hiding this comment.
From the docs it doesn't look like it can (https://docs.python.org/3/c-api/reflection.html#c.PyEval_GetBuiltins).
There was a problem hiding this comment.
The globals dict is needed, but we don't seem to be using the __builtins__ in the string. Do we really need this line?
There was a problem hiding this comment.
If you don't setup a __builtins__ or set object explicit, object may not be found.
src/runtime/runtime.cs
Outdated
| XDecref(globals); | ||
| throw new PythonException(); |
There was a problem hiding this comment.
You might want to construct an instance of PythonException before doing XDecref, otherwise last error could potentially be overwritten by XDecref with something new.
src/runtime/runtime.cs
Outdated
| @@ -1925,25 +1946,24 @@ internal static IntPtr PyMem_Realloc(IntPtr ptr, long size) | |||
|
|
|||
| internal static void SetNoSiteFlag() | |||
| { | |||
| if (_PythonDll == "__Internal") | |||
| { | |||
| throw new NotSupportedException("SetNoSiteFlag didn't support on static compile"); | |||
There was a problem hiding this comment.
Why should this not be supported? The symbol is just in the running DLL, so loader.Load(_PythonDll) has to return essentially the result of dlopen(NULL, ...).
There was a problem hiding this comment.
Yes, it should be supported. Just a mistake according to my wrong tests. I will recover it back.
|
I believe issues #891, #946, and #967 all point to a fundamental problem with this PR. On certain Linux distributions (Debian for example), Python C extension libraries are deliberately not linked to Yet .NET Core's So if this PR removes As Victor Stinner put it in Python issue issue 21536:
Unless we can get |
* Pick `SlotHelper` from pythonnet#958
|
@Jeff17Robbins This PR won't break "Embedding" mode on .NET Core, moreover it improve the compatibility since it didn't need call the |
|
Contained in #958 |
What does this implement/fix? Explain your changes.
There are some limitations to dynamic load a library on some systems(e. g. Android).
LoadLibraryjust called for once for getting the_PyObject_NextNotImplemented,we can use another ways to get this function.
Does this close any currently open issues?
No
Any other comments?
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG