Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Feb 14, 2021
Merged

New loading based on clr_loader #1373

merged 9 commits into from
Feb 14, 2021

Conversation

filmor
Copy link
Member

@filmor filmor commented Jan 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 to AUTHORS
  • Updated the CHANGELOG

@filmor filmor force-pushed the clr-loader branch 4 times, most recently from 33f969e to 54a54ce Compare January 30, 2021 19:20
lostmsu
lostmsu reviewed Jan 30, 2021
View reviewed changes
@lostmsu
Copy link
Member

lostmsu commented Jan 30, 2021 •
edited
Loading

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

@filmor
Copy link
Member Author

filmor commented Jan 30, 2021

I see failures regarding NativeTypeOffset. 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.

@filmor
Copy link
Member Author

filmor commented Jan 31, 2021

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 filmor marked this pull request as ready for review February 1, 2021 09:44
@filmor
Copy link
Member Author

filmor commented Feb 1, 2021

The finalizer breakages are from objects generated in test_class.py and test_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 the Shutdown function, that seems to have gotten lost somewhere during refactoring before.

@filmor filmor requested a review from lostmsu February 2, 2021 12:58
@filmor
Copy link
Member Author

filmor commented Feb 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 filmor mentioned this pull request Feb 2, 2021
Closed
filmor
filmor commented Feb 2, 2021
View reviewed changes
lostmsu
lostmsu requested changes Feb 9, 2021
View reviewed changes
amos402
amos402 reviewed Feb 10, 2021
View reviewed changes
@filmor filmor requested a review from lostmsu February 10, 2021 18:06
@filmor
Copy link
Member Author

filmor commented Feb 11, 2021

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

lostmsu
lostmsu reviewed Feb 12, 2021
View reviewed changes
@filmor
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 and
Mono. Apart from dropping the respective old code, this requires the following
changes:
- 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 is
currently essentially a no-op until these are fixed.
@filmor
Copy link
Member Author

filmor commented Feb 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.

@filmor filmor force-pushed the clr-loader branch 2 times, most recently from e10a492 to 5d00485 Compare February 14, 2021 11:42
@filmor filmor merged commit 6510ff7 into pythonnet:master Feb 14, 2021
@filmor filmor deleted the clr-loader branch February 14, 2021 14:23
@filmor filmor mentioned this pull request Feb 14, 2021
Closed
4 tasks
lostmsu added a commit that referenced this pull request Feb 17, 2021
@lostmsu
NuGet Preview pipeline changes missed in #1373
0f5e781
@BadSingleton BadSingleton mentioned this pull request Mar 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lostmsu lostmsu lostmsu requested changes

@amos402 amos402 amos402 left review comments

@BadSingleton BadSingleton BadSingleton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@filmor @lostmsu @amos402 @BadSingleton