NotificationsYou must be signed in to change notification settings
Fork748
Star5.1k

New loading based on clr_loader#1373

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

Merged

filmor merged 9 commits intopythonnet:masterfromfilmor:clr-loader

Feb 14, 2021

Merged

New loading based on clr_loader#1373

filmor merged 9 commits intopythonnet:masterfromfilmor:clr-loader

Feb 14, 2021

Conversation

Copy link

Member

filmor commentedJan 30, 2021•
edited
Loading

What does this implement/fix? Explain your changes.

As the last required step, this allows loading a .NET Core runtime in Python. This is not tested, yet, we'll add it to the test-suite soon.

Does this close any currently open issues?

#857 #984

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

filmor force-pushed theclr-loader branch 4 times, most recently from33f969e to54a54ceCompare

January 30, 2021 19:20

lostmsu reviewed

Jan 30, 2021

View reviewed changes

src/runtime/runtime.cs OutdatedShow resolvedHide resolved

Copy link

Member

lostmsu commentedJan 30, 2021•
edited
Loading

I see failures regardingNativeTypeOffset. Ideally Python should provide this information. (not sure if it can be done in a separate PR)

Copy link

MemberAuthor

filmor commentedJan 30, 2021

I see failures regardingNativeTypeOffset. Ideally Python should provide this information. (not sure if it can be done in a separate PR)

These are false negatives. I will pass the ABI flags through, but the "m" ABI flag can be safely ignored.

Copy link

MemberAuthor

This passes essentially all tests now (just the domain reload tests on Windows and macOS are problematic, but at least for the former I can debug this myself. For macOS setting the$PATH correctly at the beginning should be sufficient.

filmor marked this pull request as ready for review

February 1, 2021 09:44

Copy link

MemberAuthor

filmor commentedFeb 1, 2021

The finalizer breakages are from objects generated intest_class.py andtest_event.py. I'll try to pinpoint this further. I'm pretty certain that these are bugs that just show up now as we now properly call theShutdown function, that seems to have gotten lost somewhere during refactoring before.

filmor requested a review fromlostmsu

February 2, 2021 12:58

Copy link

MemberAuthor

filmor commentedFeb 2, 2021

It doesn't really make sense to fix all finalization problems we are seeing in this one PR. I consider this mostly done for now, so please review. I'll squash the changes later into more palatable chunks and will work on improving the ref-count situation in a different branch.

filmor mentioned this pull request

Feb 2, 2021

Domain reload crashes when .NET object points to PythonException#1371

Closed

filmor commented

Feb 2, 2021

View reviewed changes

src/runtime/pythonexception.cs OutdatedShow resolvedHide resolved

lostmsu requested changes

Feb 9, 2021

View reviewed changes

pythonnet/__init__.pyShow resolvedHide resolved

src/runtime/classbase.cs OutdatedShow resolvedHide resolved

src/runtime/loader.cs OutdatedShow resolvedHide resolved

src/runtime/methodbinding.cs OutdatedShow resolvedHide resolved

src/runtime/pythonexception.cs OutdatedShow resolvedHide resolved

src/runtime/runtime.csShow resolvedHide resolved

amos402 reviewed

Feb 10, 2021

View reviewed changes

src/runtime/constructorbinding.cs OutdatedShow resolvedHide resolved

src/runtime/loader.cs OutdatedShow resolvedHide resolved

src/runtime/loader.csShow resolvedHide resolved

pythonnet/__init__.pyShow resolvedHide resolved

filmor requested a review fromlostmsu

February 10, 2021 18:06

Copy link

MemberAuthor

filmor commentedFeb 11, 2021

I'll move all GC related changes to a separate branch
I'll disable the finalisation call for now

lostmsu reviewed

Feb 12, 2021

View reviewed changes

src/runtime/loader.csShow resolvedHide resolved

filmor force-pushed theclr-loader branch fromd6d1a0d to1493b4dCompare

February 13, 2021 12:15

Replace clr module by loading through clr_loader

b92d929

clr_loader is based on CFFI and allows loading pythonnet on different.NET runtime implementations, currently .NET Framework, .NET Core andMono. Apart from dropping the respective old code, this requires the followingchanges:- Move libpython discovery into Python by vendoring and adjusting   `find_libpython`- Adjust the GIL handling in the startup code as CFFI releases the GIL when   calling the external function- Remove the intermittent `configure` command as it is not required   anymore- Adjust a few test-cases- Remove `_AtExit`Due to issues with the reference counts, the `atexit` callback iscurrently essentially a no-op until these are fixed.

filmor force-pushed theclr-loader branch from0d560f3 to60db14eCompare

February 13, 2021 12:37

Copy link

MemberAuthor

filmor commentedFeb 13, 2021•
edited
Loading

@lostmsu This is now squashed and doesn't contain any fiddling with ref-counts anymore. I'll merge this if there is no further concern and open a follow-up PR that activates the shutdown, where we can then try to fix the remaining issues one by one.