Binary substitution of Python.Runtime.dll becomes safe. #456
Conversation
Public constants replaced to static readonly fields. Relaxed platform binding.
|
@dmitriyse, thanks! @vmuriart, @hsoft, @brianlloyd, @tiran and @tonyroberts, please review this. |
Codecov Report
@@ Coverage Diff @@
## master #456 +/- ##
==========================================
- Coverage 77.16% 77.12% -0.04%
==========================================
Files 65 65
Lines 5543 5547 +4
Branches 889 889
==========================================
+ Hits 4277 4278 +1
- Misses 978 981 +3
Partials 288 288
Continue to review full report at Codecov.
|
|
Why do you rename all of those fields? |
|
Python.Runtime.dll consumers should not use public constants that have different values in different builds (platform, python version, etc). Otherwise when Python.Runtime.dll replaced in binary distribution, will have mismatch between constants values used in replaced Python.Runtime.dll and constant values copied to assemblies that referenced non replaced version of Python.Runtime.dll at compile time. So we needs to replace constants to static read only fields. Also we needs to honor source code backward compatibility and produce no any breaking change in an existing consumer source code. So the only way to satisfy both "source code backward compatibility" and const->fields change is to rename constants and make them internal. |
|
closed and re-opened to check on Travis CI again |
src/runtime/runtime.cs
Outdated
|
|
||
| /// <summary> | ||
| /// EntryPoint to be used in DllImport to map to correct Unicode | ||
| /// methods prior to PEP393. Only used for PY27. | ||
| /// </summary> | ||
| private const string PyUnicodeEntryPoint = "PyUnicodeUCS4_"; | ||
| #elif UCS2 | ||
| public const int UCS = 2; | ||
| public const int _UCS = 2; |
There was a problem hiding this comment.
You only changed one branch of the #if here (same further down for _pyversion etc.)
There was a problem hiding this comment.
@filmor do you mean that only one if-branch is changed to from public to internal for _UCS? @dmitriyse can you please address this?
There was a problem hiding this comment.
I fixed this and some similar problems.
src/runtime/runtime.cs
Outdated
| public const string pyversion = "3.7"; | ||
| public const string pyver = "37"; | ||
| public const string _pyversion = "3.7"; | ||
| public const string _pyver = "37"; |
There was a problem hiding this comment.
all these _pyver* above should be internal @dmitriyse
There was a problem hiding this comment.
Yes, I will fix this soon.
|
@dmitriyse Are you going to continue on this branch? I fear we have to close it at some point as it will have deviated too far from master. |
|
I will try to do update it tomorrow. Or we will close this branch. |
src/runtime/runtime.cs
Outdated
| // We needs to replace all public constants to static readonly fields to allow | ||
| // binary substitution of different Python.Runtime.dll builds in a target application. | ||
|
|
||
| public static readonly int UCS = _UCS; |
There was a problem hiding this comment.
Please change these to properties. Allows us to make this fully dynamic in the future without breaking user code.
There was a problem hiding this comment.
Yes, I will replace them to properties.
But I did not understand this:
fully dynamic in the future without breaking user code.
Probably I need some example.
There was a problem hiding this comment.
If we ever want to have a single DLL for all platforms and Python versions, the values of pyversion or UCS will have to be evaluated at runtime on startup, that's what I meant.
src/runtime/runtime.cs
Outdated
| // binary substitution of different Python.Runtime.dll builds in a target application. | ||
|
|
||
| public readonly string pyversion = _pyversion; | ||
| public readonly string pyver = _pyver; |
* Binary substitution of Python.Runtime.Dll becomes safe. * Public constants replaced to static readonly fields. Relaxed platform binding. * Static readonly fields (related to build depented values) replaced by properties.
What does this implement/fix? Explain your changes.
Public constants replaced to static readonly fields. Relaxed platform binding.
C# Compiler copies constants of assembly. This may cause to errors when
code was compiled with reference to one build of Python.Runtime.dll and then
after compile other build of Python.Runtime.dll is used.
...
Does this close any currently open issues?
No, but it work towards cross-platform nuget package #165
...
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG