-
Notifications
You must be signed in to change notification settings - Fork 747
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
Conversation
@dmitriyse, thanks! @vmuriart, @hsoft, @tonyroberts, @tiran and @cgohlke, please review this. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
72a8476
to
ccbe2c1
Compare
@dmitriyse we have a big problem, looks like one of your pull requests is causing now each appveyor case to build over 1 hour!? |
@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. |
ccbe2c1
to
0b9fe61
Compare
0b9fe61
to
9307c8d
Compare
Hi, I'm not sure if the best place to comment, but running
The reason appears to be this line in setup.py. Trying with Skipping this part (by commenting the 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 is there any PR that should precede #546? if not, let me review it ASAP. |
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? |
On topic of this PR: You can't replace |
According to this two facts: /* 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: 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 .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. |
@fractus, your use case
will not work. Currently NetStandard libraries can be used from python only if it was embedded into dotnet core 2.0 application. There is an alternative option for your case:
.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 |
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 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. |
Project can be built with only dotnet core and setup.py DEVTOOLS works correctly.
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. |
9307c8d
to
60c5f9d
Compare
60c5f9d
to
69dd229
Compare
Please review and approve this PR. It's required to other PR (especially for Valid Finalizers). |
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. |
There was a problem hiding this comment.
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
@dmitriyse this would be my next review and than valid finalizers. Can you provide correct link to "proofs of py_ssize_t sizes"?
|
@dmitriyse this PR was combined into #557 , so which one should we close/review? |
#557 Is like a branch and probably obsolete branch, please ask @fractus what he suppose to do with this PR. 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). |
@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? |
There was a problem hiding this 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.
.NET's |
@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. |
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 :) |
Ah, I see what you mean. Yes, this makes sense, I'll have a look whether I can do the corrections. |
6db872e
to
f121c44
Compare
I've added a patch that performs the conversion, care to review it as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with this Q&A:
https://stackoverflow.com/questions/1309509/correct-way-to-marshal-size-t
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.
AUTHORS
CHANGELOG