Drop LoadLibrary #880

amos402 · 2019-06-13T19:06:21Z

What does this implement/fix? Explain your changes.

There are some limitations to dynamic load a library on some systems(e. g. Android).
LoadLibrary just 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.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

filmor · 2019-06-13T20:58:33Z

I'll have to look a bit more into this. I thought that this LoadLibrary call there is not only pulling in that one symbol but that it serves as a "proxy" to enforce loading the library into memory s.t. the P/Invoke bindings work. Since your patch passes all tests (apart from the known failing one) I must have misunderstood something and will test this in our use-case to verify. Thank you already for your contribution, if this works out this will help a lot in simplifying the build process :)

lostmsu · 2019-06-13T21:02:53Z

@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 LoadLibrary and related methods. Which I intended to upstream once 2.7 is dropped (or, for example, if we had a separate 2.x branch in maintenance mode).

filmor · 2019-06-13T21:10:40Z

@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?

lostmsu · 2019-06-13T21:13:03Z

@filmor well, the LoadLibrary is called once, but all the functions are obtained using GetProcAddress/dlsym.
See losttech@672258f

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.

filmor · 2019-09-12T08:42:42Z

@lostmsu What's your conclusion on keeping LoadLibrary? Does the code using delegates run reasonably or not?

benoithudson · 2019-11-21T21:11:23Z

src/runtime/runtime.cs

            InitializePlatformData();

            IntPtr dllLocal = IntPtr.Zero;
            var loader = LibraryLoader.Get(OperatingSystem);


Seems like you should delete these lines too, and line 295 as well since you moved it below.

benoithudson

This seems a quite reasonable patch, I think we should merge it.

I've had trouble in the past with this LoadLibrary/FreeLibrary screwing up the dlopen flags.

* Drop C module dependency when getting _PyObject_NextNotImplemented * Exception details for SetNoSiteFlag

codecov-io · 2019-12-01T14:22:42Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files           1        1           
  Lines         301      301           
=======================================
  Hits          261      261           
  Misses         40       40

Flag	Coverage Δ
#setup_linux	`65.44% <ø> (ø)`	⬆️
#setup_windows	`71.42% <ø> (ø)`	⬆️

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 5f56ebc...65e209e. Read the comment docs.

amos402 · 2019-12-01T16:36:24Z

I removed the dependency of zipimport. But still have no solution for SetNoSiteFlag, maybe we should add a docs for it(like please use -S when using static compile).

lostmsu · 2019-12-03T04:38:33Z

src/runtime/runtime.cs

+            if (dllLocal == IntPtr.Zero)
            {
-                dllLocal = loader.Load(_PythonDll);
+                throw new Exception($"Cannot load {_PythonDll}");


Can you throw more informative exception? Win32Exception would automatically read last error on Windows, but I don't know what to do for *nix

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.

lostmsu · 2019-12-03T04:39:33Z

src/runtime/runtime.cs

+        private static IntPtr Get_PyObject_NextNotImplemented()
+        {
+            IntPtr globals = PyDict_New();
+            if (PyDict_SetItemString(globals, "__builtins__", PyEval_GetBuiltins()) != 0)


Can PyEval_GetBuiltins() return an error?

From the docs it doesn't look like it can (https://docs.python.org/3/c-api/reflection.html#c.PyEval_GetBuiltins).

The globals dict is needed, but we don't seem to be using the __builtins__ in the string. Do we really need this line?

If you don't setup a __builtins__ or set object explicit, object may not be found.

lostmsu · 2019-12-03T04:41:05Z

src/runtime/runtime.cs

+                XDecref(globals);
+                throw new PythonException();


You might want to construct an instance of PythonException before doing XDecref, otherwise last error could potentially be overwritten by XDecref with something new.

…en-2

filmor · 2019-12-15T14:17:42Z

src/runtime/runtime.cs

        {
+            if (_PythonDll == "__Internal")
+            {
+                throw new NotSupportedException("SetNoSiteFlag didn't support on static compile");


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, ...).

Yes, it should be supported. Just a mistake according to my wrong tests. I will recover it back.

Jeff17Robbins · 2019-12-15T18:09:30Z

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 libpython.

Yet .NET Core's [DllImport] does not load libpython with RTLD_GLOBAL.

So if this PR removes dllLocal = loader.Load(_PythonDll); from runtime.cs, where the salient line in LibraryLoader.cs is var res = dlopen(filename, RTLD_NOW | RTLD_GLOBAL);, won't this PR break Python.NET's "Embedding" mode on .NET Core?

As Victor Stinner put it in Python issue issue 21536:

...libpython must always be loaded with RTLD_GLOBAL.

Unless we can get [DllImport] to set the RTLD_GLOBAL flag, I'm afraid we need the explicit loader.Load, at least for Linux .NET Core embedding.

* Pick `SlotHelper` from pythonnet#958

amos402 · 2020-02-13T14:49:16Z

@Jeff17Robbins This PR won't break "Embedding" mode on .NET Core, moreover it improve the compatibility since it didn't need call the dlopen.

amos402 · 2020-10-09T07:29:09Z

Contained in #958

amos402 added 3 commits June 14, 2019 02:35

Drop LoadLibrary dependency

d1928dc

Explain for getting _PyObject_NextNotImplemented

f000e08

Update changelog and authors

f882400

filmor and others added 3 commits June 20, 2019 19:29

Merge branch 'master' into drop-dlopen

b7715ee

Merge remote-tracking branch 'remotes/upstream/master' into drop-dlopen

41ac665

Merge branch 'master' into drop-dlopen

c6dae9e

benoithudson reviewed Nov 21, 2019

View reviewed changes

Merge branch 'master' into drop-dlopen

653a263

amos402 force-pushed the drop-dlopen branch from 4d9c632 to 18d8da4 Compare December 1, 2019 13:35

* Load PyModuleType without LibraryLoader

b3e889b

* Drop C module dependency when getting _PyObject_NextNotImplemented * Exception details for SetNoSiteFlag

amos402 force-pushed the drop-dlopen branch from 18d8da4 to b3e889b Compare December 1, 2019 13:40

lostmsu reviewed Dec 3, 2019

View reviewed changes

amos402 mentioned this pull request Dec 14, 2019

Optimize for Runtime.InitializePlatformData #891

Closed

amos402 added 2 commits December 15, 2019 02:26

Merge remote-tracking branch 'remotes/upstream/master' into drop-dlop…

04d6dfb

…en-2

Prevent exception override

5150e61

filmor reviewed Dec 15, 2019

View reviewed changes

Rollback symbol loading for __Internal

65e209e

amos402 added 3 commits February 13, 2020 01:07

Merge branch 'master' into drop-dlopen

431d644

* Fix refcnt error

2b84394

* Pick `SlotHelper` from pythonnet#958

Merge 'master' into drop-dlopen

b7797d3

amos402 mentioned this pull request Feb 13, 2020

Add soft shutdown #958

Merged

4 tasks

amos402 closed this Oct 9, 2020

Drop LoadLibrary #880

Drop LoadLibrary #880

Uh oh!

Conversation

amos402 commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

filmor commented Jun 13, 2019

Uh oh!

lostmsu commented Jun 13, 2019

Uh oh!

filmor commented Jun 13, 2019

Uh oh!

lostmsu commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filmor commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoithudson left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amos402 commented Dec 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jeff17Robbins commented Dec 15, 2019

Uh oh!

amos402 commented Feb 13, 2020

Uh oh!

amos402 commented Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amos402 commented Jun 13, 2019 •

edited

Loading

lostmsu commented Jun 13, 2019 •

edited

Loading

codecov-io commented Dec 1, 2019 •

edited

Loading