Fix Py_Main/PySys_SetArgvEx(...) UCS4/PY3 no mem by vmuriart · Pull Request #399 · pythonnet/pythonnet
Conversation
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
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 is51.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.
@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."?
@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.
@dmitriyse your first comment scared me. You posted it just has I was finishing writing the 2nd ICustomMarshaler.
vmuriart
deleted the
fix-pymain_setargv-py3ucs4
branch
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