Binary substitution of Python.Runtime.dll becomes safe. by dmitriyse · Pull Request #456 · pythonnet/pythonnet · GitHub

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.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

Public constants replaced to static readonly fields. Relaxed platform binding.

@dmitriyse, thanks! @vmuriart, @hsoft, @brianlloyd, @tiran and @tonyroberts, please review this.

Codecov Report

Merging #456 into master will decrease coverage by 0.03%.
The diff coverage is 75%.

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

Flag	Coverage Δ
#setup_linux	75.71% <ø> (ø)	⬆️
#setup_windows	76.47% <75%> (-0.04%)	⬇️

Impacted Files	Coverage Δ
src/runtime/CustomMarshaler.cs	69.86% <100%> (ø)	⬆️
src/runtime/runtime.cs	90.58% <66.66%> (-1.05%)	⬇️

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 87eb102...77b49a2. Read the comment docs.

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

Also, you should probably already add the static properties, if possible.

You only changed one branch of the #if here (same further down for _pyversion etc.)

@filmor do you mean that only one if-branch is changed to from public to internal for _UCS? @dmitriyse can you please address this?

I fixed this and some similar problems.

all these _pyver* above should be internal @dmitriyse

Yes, I will fix this soon.

Fixed.

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

Please change these to properties. Allows us to make this fully dynamic in the future without breaking user code.

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.

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.

These two too.

… properties.

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

Binary substitution of Python.Runtime.Dll becomes safe.

9429512

Public constants replaced to static readonly fields. Relaxed platform binding.

Build fix, lost renames performed.

49edf48

typo fix.

938030c

den-run-ai closed this Apr 26, 2017

den-run-ai reopened this Apr 26, 2017

filmor requested changes Apr 26, 2017

View reviewed changes

den-run-ai requested review from filmor, tonyroberts and vmuriart April 29, 2017 04:26

den-run-ai added the waiting for user feedback label Apr 29, 2017

den-run-ai reviewed May 9, 2017

View reviewed changes

dse added 2 commits July 5, 2017 06:40

Merged latest master

0240461

Public constants replacement fixes.

2d2fe53

den-run-ai self-requested a review July 5, 2017 04:54

den-run-ai approved these changes Jul 5, 2017

View reviewed changes

filmor requested changes Jul 10, 2017

View reviewed changes

dse added 2 commits July 10, 2017 14:44

Merge

633a636

Static readonly fields (related to build depented values) replaced by…

77b49a2

… properties.

dmitriyse force-pushed the master branch from a8b2c77 to 77b49a2 Compare July 10, 2017 10:45

filmor approved these changes Jul 10, 2017

View reviewed changes

filmor merged commit 25e66f0 into pythonnet:master Jul 10, 2017

                       private const string PyUnicodeEntryPoint = "PyUnicodeUCS4_";
               #elif UCS2
-                      public const int UCS = 2;
+                      public const int _UCS = 2;

-                      public const string pyversion = "3.7";
-                      public const string pyver = "37";
+                      public const string _pyversion = "3.7";
+                      public const string _pyver = "37";

+                      // 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;

+                      // 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. by dmitriyse · Pull Request #456 · pythonnet/pythonnet · GitHub

Repository navigation

Binary substitution of Python.Runtime.dll becomes safe. - #456

Binary substitution of Python.Runtime.dll becomes safe.#456filmor merged 7 commits intopythonnet:masterpythonnet/pythonnet:masterfrom dmitriyse:masterdmitriyse/pythonnet:masterCopy head branch name to clipboard

Conversation

dmitriyse commented Apr 25, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

mention-bot commented Apr 25, 2017

Uh oh!

codecov Bot commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

filmor commented Apr 25, 2017

Uh oh!

dmitriyse commented Apr 26, 2017

Uh oh!

den-run-ai commented Apr 26, 2017

Uh oh!

filmor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitriyse commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Binary substitution of Python.Runtime.dll becomes safe.#456
filmor merged 7 commits into
pythonnet:masterfrom
dmitriyse:master

codecov Bot commented Apr 25, 2017 •
edited
Loading

filmor commented Jul 3, 2017 •
edited
Loading