FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

First chunk of modernisation changes by filmor · Pull Request #1109 · pythonnet/pythonnet · GitHub

First chunk of modernisation changes - #1109

Closed
filmor wants to merge 13 commits into
pythonnet:masterfrom
filmor:modernisation-light
Closed

First chunk of modernisation changes#1109
filmor wants to merge 13 commits into
pythonnet:masterfrom
filmor:modernisation-light

Conversation

filmor commented Apr 10, 2020

Copy link
Copy Markdown
Member
  • Move projects around and add clean project/solution files
  • Drop monoclr and clrmodule
  • Add stub clr module
  • Drop nuget config
  • Add test helper project
  • Drop PYTHON3 flag (default now)
  • Remove unneeded additional interop files
  • Make NETSTANDARD the default
    Drops also
    • Custom XDecref/XIncref (should be readded)
    • Remote object handling for .NET Framework
  • Remove Runtime._UCS dependency from CustomMarshaler
  • Stop using Runtime.IsPython2/3

What does this implement/fix? Explain your changes.

...

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 to AUTHORS
  • Updated the CHANGELOG

- Move projects around and add clean project/solution files
- Drop monoclr and clrmodule
- Add stub clr module
- Drop nuget config
- Add test helper project
- Drop PYTHON3 flag (default now)
- Remove unneeded additional interop files
- Make NETSTANDARD the default
    Drops also
    - Custom XDecref/XIncref (should be readded)
    - Remote object handling for .NET Framework
- Remove Runtime._UCS dependency from CustomMarshaler
- Stop using Runtime.IsPython2/3

dnfclas commented Apr 10, 2020
edited
Loading

Copy link
Copy Markdown


All CLA requirements met.

filmor force-pushed the modernisation-light branch from 3403bb6 to 52e4f15 Compare April 10, 2020 16:44
filmor added 4 commits April 11, 2020 18:36
- Swapped UCS2 in one place
- Explicitly acquire GILState in InternalInitialize
- Fix a few problems with short-ints by using larger numbers
  (`PyLongType`) or dropping the respective return (`AtExit`)
filmor force-pushed the modernisation-light branch from 3862a0d to 054b872 Compare April 11, 2020 22:24

filmor commented Apr 12, 2020

Copy link
Copy Markdown
Member Author

@koubaa @lostmsu I've gotten quite a bit further. The current state of this PR implements embedding in .NET Core and Mono on Linux (haven't tested yet on Windows, will not be able to test on OS X), I tested with the following script:

import os
#os.environ["MONO_LOG_LEVEL"] = "debug"
#os.environ["MONO_LOG_MASK"] = "cfg,dll"
#os.environ["COREHOST_DEBUG"] = "1"
#os.environ["COREHOST_TRACE"] = "1"
#os.environ["COREHOST_TRACE_VERBOSITY"] = "4"

import pythonnet, clr_loader

import sys

if sys.argv[1] == "mono":
    mono = clr_loader.get_mono()
    rt = mono
elif sys.argv[1] == "core":
    core = clr_loader.get_coreclr("/home/benedikt/git/clr-loader/example/out/example.runtimeconfig.json")
    rt = core

pythonnet.set_runtime(rt)
pythonnet.load()

print("Loaded pythonnet")

import clr
from System import Console
Console.WriteLine("Success")

Still crashes when I try to do this from IPython, but we're getting somewhere :)

Next steps are:

  • Make a clr.py that handles these initial load steps like they worked before and make the test-suite work
  • See how this works for the other direction (embedding Python in C#)
  • Test on Windows in all variants
  • Allow for a simpler way to configure .NET Core (currently requires a runtimeconfig.json)

koubaa commented Apr 12, 2020
edited
Loading

Copy link
Copy Markdown
Contributor

@filmor this PR seems to accomplish a lot, I wish it could be staged so it would be easier to follow. For instance, does this also fix 857 and drop support for python2?

Also the file renames I think should be done in a separate PR to better understand the scope. If you like I can take care of adding some of these easier stages.

sidenote I totally agree with dropping remoting support. If someone wants that they should maintain a fork.

filmor commented Apr 12, 2020

Copy link
Copy Markdown
Member Author

This doesn't drop Python 2 support, yet, it merely defaults to Python 3 unless PYTHON2 is defined. I don't think moving around the files and sticking to the old csproj file format makes sense. Regarding stages, be my guest, I just don't want to redo all parts of this multiple times. Moving to the new project format only really works while simultaneously dropping the old Mono CLR loader, which then requires the new one to be in place for tests to be able to run.

What I want to accomplish in the end here is:

  1. New project format
  2. Use of clr-loader
  3. .NET Standard only
  4. Reasonable structure for the interop*.cs files

koubaa commented Apr 13, 2020

Copy link
Copy Markdown
Contributor

@filmor I'll push up a PR with some things staged. I think the smaller and more incremental we can make this branch the easier it'll be to land (not to mention to allow users to bisect if it breaks anyone!)

My first PR will be:

  • default to python 3
  • drop custom xincref/xdecref & remoting
  • Stop using Runtime.IsPython2/3

Which variants do we need to test on windows? I can also work on that + embedding.

filmor commented Apr 13, 2020

Copy link
Copy Markdown
Member Author

@koubaa Have a look at the other PR (#1015), it has much smaller grained commits. The reason I went for this larger variant is that I did some steps in the "wrong" order (first moving the files around, then modifying them).

The parts you are suggesting may safely go against master, all steps that change the project structure can't. Please ensure that they are compatible with this branch, though.

I don't understand what you mean by "variants".

koubaa commented Apr 15, 2020

Copy link
Copy Markdown
Contributor

@filmor I am referring to a task in your comment 3 days ago: "Test on Windows in all variants"

filmor commented Apr 15, 2020

Copy link
Copy Markdown
Member Author

On Windows we can use all .NET runtimes, i.e. Framework, Core and Mono, so all of those should be tested.

koubaa commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

@filmor understood. I think I can manage that. In the meantime I pushed the first stage here: #1117

filmor force-pushed the modernisation-light branch from 3d8416b to 1adc2ff Compare May 10, 2020 14:13
IntPtr gilState = IntPtr.Zero;
try
{
gilState = Runtime.PyGILState_Ensure();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does this have to be inside try block?

<Project Sdk="Microsoft.NET.Test.Sdk">

<PropertyGroup>
<TargetFrameworks>netcoreapp3.0;net472</TargetFrameworks>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

netcoreapp3.0 will soon be unsupported. We should use netcoreapp3.1 which is LTS

</ItemGroup>

<ItemGroup Condition="$(TargetFramework) == 'net472'">
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0-preview.2">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think non-preview version of this package was released.

filmor closed this Jan 29, 2021
filmor deleted the modernisation-light branch January 29, 2021 06:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL