@vmuriart will this cherry pick include @dmitriyse in the commit history?

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?

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

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

@filmor good call. Fixed and pushed.

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

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.

@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.

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?

@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.

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

The last two are a bit of a stretch, but still related.

https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L1511
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L1664
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L1731
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L1799
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/converter.cs#L609
https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L923

@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?

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.

Ups, i am worng.
https://github.com/dotnet/corefx/issues/1346
ICustomMarshaler is coming back in NetStandard 2.0
So we can use it. Everybody will migrate to NetStandard 2.0 after this march.

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