Skip to content

Interop methods with Py_ssize_t works differently in NetCoreApp 2.0 #531

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

dmitriyse
Copy link
Contributor

What does this implement/fix? Explain your changes.

Interop methods in the NetCoreApp 2.0 is more sensitive than in the net40.
All interop methods containing py_ssize_t are wrongly works with negative numbers under x64 platform under NetCoreApp 2.0.
This change fixes the problem.
...

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

@mention-bot
Copy link

@dmitriyse, thanks! @vmuriart, @hsoft, @tonyroberts, @tiran and @cgohlke, please review this.

@codecov
Copy link

codecov bot commented Aug 28, 2017

Codecov Report

Merging #531 into master will decrease coverage by 0.07%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #531      +/-   ##
==========================================
- Coverage   77.15%   77.08%   -0.08%     
==========================================
  Files          63       63              
  Lines        5695     5729      +34     
  Branches      904      904              
==========================================
+ Hits         4394     4416      +22     
- Misses       1004     1016      +12     
  Partials      297      297
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
#setup_windows 76.27% <80%> (-0.07%) ⬇️
Impacted Files Coverage Δ
src/runtime/methodbinding.cs 79.59% <100%> (ø) ⬆️
src/runtime/methodbinder.cs 90.72% <100%> (ø) ⬆️
src/runtime/converter.cs 81.81% <100%> (ø) ⬆️
src/runtime/importhook.cs 79.22% <100%> (ø) ⬆️
src/runtime/classobject.cs 74.19% <100%> (ø) ⬆️
src/runtime/metatype.cs 64.65% <100%> (ø) ⬆️
src/runtime/assemblymanager.cs 89.1% <100%> (ø) ⬆️
src/runtime/arrayobject.cs 77% <100%> (ø) ⬆️
src/runtime/interfaceobject.cs 50% <100%> (ø) ⬆️
src/runtime/indexer.cs 85.71% <100%> (ø) ⬆️
... and 3 more

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 08344b7...1e41038. Read the comment docs.

@dmitriyse dmitriyse force-pushed the py_ssize_t-fix branch 3 times, most recently from 72a8476 to ccbe2c1 Compare September 4, 2017 09:30
@den-run-ai
Copy link
Contributor

@dmitriyse we have a big problem, looks like one of your pull requests is causing now each appveyor case to build over 1 hour!?

image

@den-run-ai
Copy link
Contributor

@dmitriyse i cancelled bunch of older builds, hopefully this should help.

@dmitriyse
Copy link
Contributor Author

@dmitriyse i cancelled bunch of older builds, hopefully this should help.

Big thanks. I just wanted to asked for it.

I faced with weird mono unstability. Now I know the reason, but problem identification was the real pain.
Sorry that I flooded the Appveyor.

@fractus
Copy link
Contributor

fractus commented Sep 28, 2017

Hi, I'm not sure if the best place to comment, but running python setup.py install in this PR, on a Win10 machine with only dotnetcore 2.0 and no VS, it seems to pick msbuild from the .NET framework runtime; here's the output:

(dotnet2) username@host c:\temp\pythonnet
> python setup.py build
running build
running build_ext
Checking for updates from https://www.nuget.org/api/v2/.
Currently running NuGet.exe 4.1.0.
Updating NuGet.exe to 4.3.0.
Update successful.
MSBuild auto-detection: using msbuild version '4.0' from 'C:\Windows\Microsoft.NET\Framework64\v4.0.30319'.
All packages listed in packages.config are already installed.
Traceback (most recent call last):
  File "setup.py", line 502, in <module>
    zip_safe=False,
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\core.py", line 148, in setup
    dist.run_commands()
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\dist.py", line 955, in run_commands
    self.run_command(cmd)
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\dist.py", line 974, in run_command
    cmd_obj.run()
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\command\build.py", line 135, in run
    self.run_command(cmd_name)
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\dist.py", line 974, in run_command
    cmd_obj.run()
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\command\build_ext.py", line 339, in run
    self.build_extensions()
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\command\build_ext.py", line 448, in build_extensions
    self._build_extensions_serial()
  File "c:\tools\Anaconda3\envs\dotnet2\lib\distutils\command\build_ext.py", line 473, in _build_extensions_serial
    self.build_extension(ext)
  File "setup.py", line 251, in build_extension
    manifest = self._get_manifest(dest_dir)
  File "setup.py", line 269, in _get_manifest
    mt = self._find_msbuild_tool("mt.exe", use_windows_sdk=True)
  File "setup.py", line 367, in _find_msbuild_tool
    raise RuntimeError("{0} could not be found".format(tool))
RuntimeError: mt.exe could not be found

The reason appears to be this line in setup.py.

Trying with DEVTOOLS = 'dotnet', it fails further down because of pkg-config due to that line.

Skipping this part (by commenting the DEVTOOLS === 'dotnet') part, the build succeeds, but the installation is not complete; python -c "import clr" fails now, as there's not clr.pyd.

Alternatively, I used the wheel file from appveyor, which succesfully install everything. However, now, despite the fact that system DLLs can be imported in Python, a simple hello-world library (netstandard2.0) called M22 fails with the following:

> ipython
Python 3.6.1 |Anaconda custom (64-bit)| (default, May 11 2017, 13:25:24) [MSC v.1900 64 bit (AMD64)]
Type 'copyright', 'credits' or 'license' for more information
IPython 6.2.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import clr

In [2]: clr.AddReference('System')
Out[2]: <System.Reflection.RuntimeAssembly at 0x23cb30f5e48>

In [3]: from System import DateTime

In [4]: print(DateTime.Now)
28/09/2017 10:22:58

In [5]: m22 = clr.AddReference('M22')

In [6]: [str(a) for a in m22.get_ExportedTypes()]
---------------------------------------------------------------------------
FileNotFoundException                     Traceback (most recent call last)
<ipython-input-6-cf6f2e420cd8> in <module>()
----> 1 [str(a) for a in m22.get_ExportedTypes()]

FileNotFoundException: Could not load file or assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.
   at System.Reflection.RuntimeAssembly.GetExportedTypes(RuntimeAssembly assembly, ObjectHandleOnStack retTypes)
   at System.Reflection.RuntimeAssembly.GetExportedTypes()

All the above is run on a clean Python 3.6 conda environment. This also fails on master. Happy to investigate further, if this an actual problem.

Ps. The above works fine with VS and .NET Framework.

@dmitriyse
Copy link
Contributor Author

@fractus, Hi!.
This problem is already solved but not included yet in this PR.
See PR #546

Please try to merge those two PR's.

@den-run-ai
Copy link
Contributor

@dmitriyse is there any PR that should precede #546? if not, let me review it ASAP.

@dmitriyse
Copy link
Contributor Author

dmitriyse commented Sep 28, 2017

No, it's a fix pack after two merges of PR #518 and #519. Fixes for review issues

@fractus
Copy link
Contributor

fractus commented Sep 29, 2017

Thanks @dmitriyse. It appears #546 has same behaviour as described above. I will look into it further over the weekend, just want to clarify what you mean: I should try to merge #531 and #546?

@filmor
Copy link
Member

filmor commented Sep 29, 2017

On topic of this PR: You can't replace size_t by IntPtr, that is just not the same type, it's platform dependent. Where did you actually see this problem? Is using uint instead an option?

@dmitriyse
Copy link
Contributor Author

dmitriyse commented Sep 29, 2017

On topic of this PR: You can't replace size_t by IntPtr, that is just not the same type, it's platform dependent. Where did you actually see this problem? Is using uint instead an option?

According to this two facts:
Python headers:

/* Py_ssize_t is a signed integral type such that sizeof(Py_ssize_t) ==
 * sizeof(size_t).  C99 doesn't define such a thing directly (size_t is an
 * unsigned integral type).  See PEP 353 for details.
 */
#ifdef HAVE_SSIZE_T
typedef ssize_t         Py_ssize_t;
#elif SIZEOF_VOID_P == SIZEOF_SIZE_T
typedef Py_intptr_t     Py_ssize_t;
#else
#   error "Python needs a typedef for Py_ssize_t in pyport.h."
#endif

And information from the:
https://www.python.org/dev/peps/pep-0353/

IntPtr always corresponds to Py_ssize_t

For example, problems occurs on a 64bit python, when .Net wants to pass negative number through System.Int32 to the Py_ssize_t argument.
On .Net framework it's works due some bug or magical FFFFFFF fulfillment of high 32 bit part of Py_ssize_t in stack

On .NetStandard 2.0 no any magic performed and python receives 0x00000000FFFFFFFF instead of 0xFFFFFFFFFFFFFFFF for -1 value.

You can reproduce this problem in the src/embed_tests/TestPySequence.cs -> TestRepeat test.

@dmitriyse
Copy link
Contributor Author

dmitriyse commented Oct 3, 2017

@fractus, your use case

> ipython
Python 3.6.1 |Anaconda custom (64-bit)| (default, May 11 2017, 13:25:24) [MSC v.1900 64 bit (AMD64)]
Type 'copyright', 'credits' or 'license' for more information
IPython 6.2.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import clr

In [2]: clr.AddReference('System')
Out[2]: <System.Reflection.RuntimeAssembly at 0x23cb30f5e48>

In [3]: from System import DateTime

In [4]: print(DateTime.Now)
28/09/2017 10:22:58

In [5]: m22 = clr.AddReference('M22')

In [6]: [str(a) for a in m22.get_ExportedTypes()]
---------------------------------------------------------------------------
FileNotFoundException                     Traceback (most recent call last)
<ipython-input-6-cf6f2e420cd8> in <module>()
----> 1 [str(a) for a in m22.get_ExportedTypes()]

FileNotFoundException: Could not load file or assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.
   at System.Reflection.RuntimeAssembly.GetExportedTypes(RuntimeAssembly assembly, ObjectHandleOnStack retTypes)
   at System.Reflection.RuntimeAssembly.GetExportedTypes()

will not work.

Currently NetStandard libraries can be used from python only if it was embedded into dotnet core 2.0 application.
So you should use "dotnet nPython.dll" and then try to import M22

There is an alternative option for your case:

  1. Try to install .Net Framework 4.7.1+
  2. If it's not helps, try to change targets from net40 to net47 in the runtime.csproj and dependent files
    and then rebuild pythonnet

.Net Framework 4.7.1+ supports to load netstandard2.0 libraries.

It's should be possible also on linux with latest mono 5.2+ (try to use alpha channel)

See #96 issue. Embedding .net core into python currently not supported, until DllExport will add valid .NetCore support

@fractus
Copy link
Contributor

fractus commented Oct 3, 2017

Thanks @dmitriyse. From what I see, the project cannot be build with only dotnetcore due to the mono includes in pynetclr.h.

Also, I'm not sure why the DEVTOOLS in setup.py is updated to dotnet for the mono case.

Last, I have merged #546 and #531 and tested them in Win7/Win10 with Python 3.6. I will also check against macOS, and then submit a PR.

@dmitriyse
Copy link
Contributor Author

Thanks @dmitriyse. From what I see, the project cannot be build with only dotnetcore due to the mono includes in pynetclr.h.
Also, I'm not sure why the DEVTOOLS in setup.py is updated to dotnet for the mono case.

Project can be built with only dotnet core and setup.py DEVTOOLS works correctly.
Currently we have two build systems

  1. Classic - based on mono(linux) and .Net Framework(windows).
  2. Xplat - based on .Net Core msbuild

try to build pythonnet with this like command

python setup.py build_ext --xplat --inplace

DEVTOOLS = "dotnet" for mono case only when --xplat option specified. And dotnet really builds mono version.

@dmitriyse
Copy link
Contributor Author

dmitriyse commented Jan 15, 2018

Please review and approve this PR. It's required to other PR (especially for Valid Finalizers).
Wrong mapping of py_ssize_t causes floating errors on Windows x86.
See this comment https://github.com/dmitriyse with proofs of py_ssize_t sizes on different python builds.

CHANGELOG.md Outdated
@@ -21,6 +21,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].

### Fixed

- Fixed interop methods with Py_ssize_t. NetCoreApp 2.0 is more sensitive than net40 and requires this fix.
Copy link
Contributor

Choose a reason for hiding this comment

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

please add ref to this PR

@den-run-ai
Copy link
Contributor

@dmitriyse this would be my next review and than valid finalizers. Can you provide correct link to "proofs of py_ssize_t sizes"?

Please review and approve this PR. It's required to other PR (especially for Valid Finalizers).
Wrong mapping of py_ssize_t causes floating errors on Windows x86.
See this comment https://github.com/dmitriyse with proofs of py_ssize_t sizes on different python builds.

@den-run-ai
Copy link
Contributor

@dmitriyse this PR was combined into #557 , so which one should we close/review?

@dmitriyse
Copy link
Contributor Author

#557 Is like a branch and probably obsolete branch, please ask @fractus what he suppose to do with this PR.
This request is totally independent and is required only for "valid finalizers".

Proof: this test should Fail under CoreClr 2.0

        [Test]
        public void TestRepeat()
        {
            var t1 = new PyString("Foo");

            PyObject actual = t1.Repeat(3);
            Assert.AreEqual("FooFooFoo", actual.ToString());

            actual = t1.Repeat(-3);
            Assert.AreEqual("", actual.ToString());
        }

Also random bufs occurs in massive test execution (at least in .Net Core 2.0).
More detailed explanation I provided below (#531 (comment))

@fractus
Copy link
Contributor

fractus commented Feb 14, 2018

I think #557 can be closed since it's quite behind master and try to merge this first; you can include the typo from this commit.

@den-run-ai den-run-ai mentioned this pull request Jun 24, 2018
4 tasks
@filmor filmor changed the title Interpo methods with Py_ssize_t works differently in NetCoreApp 2.0 and requires interop fix. Intero methods with Py_ssize_t works differently in NetCoreApp 2.0 Oct 18, 2018
@filmor filmor changed the title Intero methods with Py_ssize_t works differently in NetCoreApp 2.0 Interop methods with Py_ssize_t works differently in NetCoreApp 2.0 Oct 18, 2018
@filmor
Copy link
Member

filmor commented Oct 18, 2018

@dmitriyse I stand corrected, you are completely right. From my perspective, if the tests go through, this one is good to merge. @denfromufa any concerns?

Copy link
Contributor

@tonyroberts tonyroberts left a comment

Choose a reason for hiding this comment

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

I think it would be better to use long everywhere instead of int. The IntPtr conversion will throw on 32 bit systems when necessary, and otherwise it won't be possible to index the full range possible in Python if left as int.

@filmor
Copy link
Member

filmor commented Oct 19, 2018

.NET's long is not right, as it is universally 64bit. The Py_ssize_t type is ssize_t if available and otherwise Py_intptr_t, so it is in all relevant cases the same size as a pointer. The only case where this is not true is for weird architectures with segmented pointers of some sort, so IntPtr is in my opinion the right (or at least closest) type for everything that is Py_ssize_t in the Python API.

@tonyroberts
Copy link
Contributor

tonyroberts commented Oct 19, 2018

@filmor yep, I agree with that - sorry, I wasn't very clear. I meant that everywhere an IntPtr is being converted to/from an int (eg where PySequence_GetItem takes an int as the index), wouldn't it be better to use long there? Calling it with a number greater than 32 bits on a 32 bit system would cause an exception, but at least it would be possible to index the full range on 64 bit systems.

@tonyroberts
Copy link
Contributor

Although I see previously they were also taking ints, so this PR doesn't affect that behaviour... so maybe it's fine to leave it and make that int->long change in another PR :)

@filmor
Copy link
Member

filmor commented Oct 19, 2018

Ah, I see what you mean. Yes, this makes sense, I'll have a look whether I can do the corrections.

@filmor
Copy link
Member

filmor commented Oct 19, 2018

I've added a patch that performs the conversion, care to review it as well?

@filmor filmor added the next label Oct 19, 2018
@den-run-ai den-run-ai self-requested a review October 20, 2018 04:57
Copy link
Contributor

@den-run-ai den-run-ai left a comment

Choose a reason for hiding this comment

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

@filmor filmor merged commit e5d299e into pythonnet:master Oct 23, 2018
@filmor filmor mentioned this pull request Oct 30, 2019
4 tasks
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.

6 participants