NotificationsYou must be signed in to change notification settings
Fork768
Star5.4k

Drop LoadLibrary#880

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

Jump to bottom

Closed

amos402 wants to merge14 commits intopythonnet:masterfromamos402:drop-dlopen

Closed

Drop LoadLibrary#880

amos402 wants to merge14 commits intopythonnet:masterfromamos402:drop-dlopen

Conversation

Copy link

Member

amos402 commentedJun 13, 2019•
edited
Loading

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?

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 toAUTHORS
Updated theCHANGELOG

amos402 added3 commits

June 14, 2019 02:35

Drop LoadLibrary dependency

d1928dc

Explain for getting _PyObject_NextNotImplemented

f000e08

Update changelog and authors

f882400

Copy link

Member

filmor commentedJun 13, 2019

I'll have to look a bit more into this. I thought that thisLoadLibrary 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 :)

Copy link

Member

lostmsu commentedJun 13, 2019

@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 ofLoadLibrary 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).

Copy link

Member

filmor commentedJun 13, 2019

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

Copy link

Member

lostmsu commentedJun 13, 2019•
edited
Loading

@filmor well, theLoadLibrary is called once, but all the functions are obtained usingGetProcAddress/dlsym.
Seelosttech@672258f

And alsolosttech@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.

filmorand others added3 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

Copy link

Member

filmor commentedSep 12, 2019

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

benoithudson reviewed

Nov 21, 2019

View reviewed changes

src/runtime/runtime.cs Outdated

		InitializePlatformData();

		IntPtrdllLocal=IntPtr.Zero;
		varloader=LibraryLoader.Get(OperatingSystem);

Copy link

Contributor

benoithudsonNov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

benoithudson reviewed

Nov 21, 2019

View reviewed changes

Copy link

Contributor

benoithudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Merge branch 'master' into drop-dlopen

653a263

amos402 force-pushed thedrop-dlopen branch from4d9c632 to18d8da4Compare

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 thedrop-dlopen branch from18d8da4 tob3e889bCompare

December 1, 2019 13:40

Copy link

codecov-io commentedDec 1, 2019•
edited
Loading

Codecov Report

Merging#880 intomaster willnot change coverage.
The diff coverage isn/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 byCodecov. Last update5f56ebc...65e209e. Read thecomment docs.

Copy link

MemberAuthor

amos402 commentedDec 1, 2019

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

lostmsu reviewed

Dec 3, 2019

View reviewed changes

src/runtime/runtime.cs Outdated

		if(dllLocal== IntPtr.Zero)
		{
		dllLocal= loader.Load(_PythonDll);
		thrownew Exception($"Cannot load{_PythonDll}");

Copy link

Member

lostmsuDec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

amos402Dec 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Generally it should throw inside fromLoad, 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

		privatestaticIntPtrGet_PyObject_NextNotImplemented()
		{
		IntPtrglobals=PyDict_New();
		if(PyDict_SetItemString(globals,"__builtins__",PyEval_GetBuiltins())!=0)

Copy link

Member

lostmsuDec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

CanPyEval_GetBuiltins() return an error?

Copy link

Member

filmorDec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

benoithudsonDec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

amos402Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you don't setup a__builtins__ or setobject explicit,object may not be found.

src/runtime/runtime.cs Outdated

Comment on lines 335 to 336

		XDecref(globals);
		thrownewPythonException();

Copy link

Member

lostmsuDec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You might want to construct an instance ofPythonException before doingXDecref, otherwise last error could potentially be overwritten byXDecref with something new.

amos402 mentioned this pull request

Dec 14, 2019

Optimize forRuntime.InitializePlatformData#891

Closed

amos402 added2 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

src/runtime/runtime.cs Outdated

		{
		if(_PythonDll== "__Internal")
		{
		thrownew NotSupportedException("SetNoSiteFlag didn't support onstatic compile");

Copy link

Member

filmorDec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why should this not be supported? The symbol is just in the running DLL, soloader.Load(_PythonDll) has to return essentially the result ofdlopen(NULL, ...).

Copy link

MemberAuthor

amos402Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Rollback symbol loading for __Internal

65e209e

Copy link

Contributor

Jeff17Robbins commentedDec 15, 2019

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 librariesare deliberately not linked tolibpython.

Yet .NET Core's[DllImport] does not loadlibpython withRTLD_GLOBAL.

So if this PR removesdllLocal = loader.Load(_PythonDll); fromruntime.cs, where the salient line inLibraryLoader.cs isvar 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 inPython issue issue 21536:

...libpython must always be loaded with RTLD_GLOBAL.

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

amos402 added3 commits

February 13, 2020 01:07

Merge branch 'master' into drop-dlopen

431d644

* Fix refcnt error

2b84394

* Pick `SlotHelper` frompythonnet#958

Merge 'master' into drop-dlopen

b7797d3

Copy link

MemberAuthor

amos402 commentedFeb 13, 2020

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

amos402 mentioned this pull request

Feb 13, 2020

Add soft shutdown#958

Merged

4 tasks

Copy link

MemberAuthor

amos402 commentedOct 9, 2020

Contained in#958

amos402 closed this

Oct 9, 2020

Labels

None yet

Movatterモバイル変換

Drop LoadLibrary#880

Drop LoadLibrary#880

Uh oh!

Conversation

amos402 commentedJun 13, 2019• editedLoading 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 commentedJun 13, 2019

Uh oh!

lostmsu commentedJun 13, 2019

Uh oh!

filmor commentedJun 13, 2019

Uh oh!

lostmsu commentedJun 13, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

filmor commentedSep 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 commentedDec 1, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amos402 commentedDec 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 commentedDec 15, 2019

Uh oh!

amos402 commentedFeb 13, 2020

Uh oh!

amos402 commentedOct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amos402 commentedJun 13, 2019•
edited
Loading

lostmsu commentedJun 13, 2019•
edited
Loading

codecov-io commentedDec 1, 2019•
edited
Loading