Fix Py_Main/PySys_SetArgvEx(...) UCS4/PY3 no mem by vmuriart · Pull Request #399 · pythonnet/pythonnet

Conversation

@vmuriart

What does this implement/fix? Explain your changes.

Fix Py_Main & PySys_SetArgvEx execution on PY3/UCS4 (Linux).

Does this close any currently open issues?

N/A

Any other comments?

Based on @dmitriyse's work.
dmitriyse@8a70f09

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • Add yourself to AUTHORS

@den-run-ai

@vmuriart

I didn't cherry pick this commit in this case. He had re-organized/re-structured his files and was going to be a headache to cherry pick. I gave him credit on the commit message though. @dmitriyse are you ok w this?

@filmor

You can just do a git commit --amend --author "Something <something>" to give credit :)

Could you or @dmitriyse explain why this fixes the issue?

@vmuriart

@filmor good call. Fixed and pushed.

@codecov

Codecov Report

Merging #399 into master will decrease coverage by -0.09%.
The diff coverage is 51.42%.

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
- Coverage   63.53%   63.45%   -0.09%     
==========================================
  Files          60       60              
  Lines        5222     5257      +35     
  Branches      858      860       +2     
==========================================
+ Hits         3318     3336      +18     
- Misses       1684     1701      +17     
  Partials      220      220
Flag Coverage Δ
#Embedded_Tests 33% <51.42%> (+0.12%)
#Python_Tests 60.07% <51.42%> (-0.42%)
#Setup_Linux 74.5% <51.42%> (ø)
#Setup_Windows 70.5% <51.42%> (ø)
Impacted Files Coverage Δ
src/runtime/runtime.cs 80.74% <51.42%> (-3.94%)

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 b4ed645...9b70892. Read the comment docs.

@vmuriart

@filmor I'll let @dmitriyse since he figured it out. My best educated guess is that the encoding was breaking for UCS4 and thus why we only saw the issue on linux.

@den-run-ai

This code definitely needs comments on how these unmanaged strings are manually built.

I'm surprised that it has to be so convoluted. Why can't we just use PyString_* methods?

@vmuriart

@denfromufa I have some work to refactor that logic. We have about 3 different variations of this same thing, each variation being used atleast twice. When I tried refactoring earlier, I was having issues that either the memory was being freed-up too early or never at all. We do the same thing on PyString_* as this cuz of the same memory life issue.

I settled for this pr until I get the memory-life tuned just right.

@den-run-ai

@vmuriart can you point to "3 different variations of this same thing, each variation being used atleast twice."?

@vmuriart

@vmuriart

@filmor I took a closer look at the issue and I was on the right track. Currently we don't distinguish between the two different encoding, and fails to work on UCS4. This implementation manually marshals the string array and applies to correct encoding passing it to python.

I'm working on an ICustomMarshaler to replace all instances that were are doing this, but I would prefer to have time one in the git-history so that we can fall-back to in case the ICustomMarshaler causes issues.

that being said, @filmor @denfromufa any objections to merging this then?

@dmitriyse

Hi! There is a problem with ICustomMarshaler - it's not supported by CoreCLR. Please consider to use two Methods solution: First Method - is interop; Second method - is interop wrapper with marshaling inside.
If we want to have single code base and avoid lots of merges and cherry picks. We needs to think in terms of the future adoptions.

@dmitriyse

@vmuriart

@dmitriyse your first comment scared me. You posted it just has I was finishing writing the 2nd ICustomMarshaler.

@vmuriart vmuriart deleted the fix-pymain_setargv-py3ucs4 branch

February 26, 2017 01:45

Labels