Skip to content

Added side-by-side VS 2017 build with NetStandard 1.5 support. #444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from

Conversation

dmitriyse
Copy link
Contributor

@dmitriyse dmitriyse commented Mar 28, 2017

What does this implement/fix? Explain your changes.

Added new files that allows to build PythonNet targeting NetStarndard 1.5
Current build scripts are not affected.

All code modification should be loss-less for current build configurations.

New VS 2017 build files are capable to build net40 and NetStandard 1.5 assemblies.

This change is a first CoreCLR compilable version. Probably it is not functional yet.
The work is in progress.

Also CoreCLR assembly/package now have 3.0.0-alpha version. Probably it's ok, if PythonNet with CoreCLR support is scheduled to 3.0.0
...

Does this close any currently open issues?

Not closes yet but relates to #96
...

Any other comments?

We can join this branch into the master even now, and continue to develop with the additional target (NetStandard 1.5).
But it is reasonable to do it after the point where both Windows and Linux configurations will be tested.
...

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

@mention-bot
Copy link

Unable to parse mention-bot custom configuration file due to a syntax error.
Please check the potential root causes below:

  1. Having comments
  2. Invalid JSON type
  3. Having extra "," in the last JSON attribute

Error message:

Error: Parse error on line 2:
... "maxReviewers": 5, // Maximum  number o
-----------------------^
Expecting 'STRING', got 'undefined'

@mention-bot
Copy link

@dmitriyse, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vmuriart, @tonyroberts and @tiran to be potential reviewers.

@dmitriyse
Copy link
Contributor Author

I will inject ReflectionBridge right into PythonNet, to save single-Dll deployments.

@codecov
Copy link

codecov bot commented Mar 28, 2017

Codecov Report

Merging #444 into master will decrease coverage by 10.47%.
The diff coverage is 59.7%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #444       +/-   ##
===========================================
- Coverage   77.03%   66.56%   -10.48%     
===========================================
  Files          62       63        +1     
  Lines        5526     5270      -256     
  Branches      886      843       -43     
===========================================
- Hits         4257     3508      -749     
- Misses        981     1506      +525     
+ Partials      288      256       -32
Flag Coverage Δ
#setup_linux 74.76% <ø> (-0.96%) ⬇️
#setup_windows 65.88% <59.7%> (-10.51%) ⬇️
Impacted Files Coverage Δ
src/runtime/interfaceobject.cs 50% <ø> (ø) ⬆️
src/runtime/codegenerator.cs 80% <ø> (ø) ⬆️
src/runtime/genericutil.cs 85.93% <ø> (ø) ⬆️
src/runtime/debughelper.cs 0% <ø> (ø) ⬆️
src/runtime/delegateobject.cs 58.53% <ø> (-7.38%) ⬇️
src/runtime/importhook.cs 78.01% <ø> (-2.41%) ⬇️
src/runtime/methodwrapper.cs 85.71% <ø> (ø) ⬆️
src/runtime/propertyobject.cs 70% <ø> (ø) ⬆️
src/runtime/metatype.cs 69.1% <ø> (-2.45%) ⬇️
src/runtime/eventobject.cs 78.2% <ø> (ø) ⬆️
... and 45 more

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 25e66f0...45eaa72. Read the comment docs.

@dmitriyse dmitriyse force-pushed the coreclr branch 2 times, most recently from 7ec0c02 to 563a844 Compare March 30, 2017 23:22
@vmuriart
Copy link
Contributor

vmuriart commented Apr 2, 2017

Alot of your changes are also changing the encoding of the files.
I'll review this in more detail, but I think there's still room to improve compatibility to avoid having multiple #if NETSTANDARD 1.5 sections.

@den-run-ai
Copy link
Contributor

@vmuriart did you have a chance to review this .net core integration?

@vmuriart
Copy link
Contributor

Very much against it in the current form.

  • Adds too many build directives, making the code complicated. We just removed a bunch of them for PY2/PY3, don't want to readd them now for this
  • No new CI added for .net core. If something is added and breaks we won't know.

A better path would be to either refactor some more or completely move to it.

@den-run-ai
Copy link
Contributor

den-run-ai commented Apr 24, 2017 via email

@vmuriart vmuriart self-requested a review April 25, 2017 02:01
dse added 5 commits April 25, 2017 21:38
@den-run-ai
Copy link
Contributor

@dmitriyse can we close this pull request, or is this still part of other pull requests for .NET Core support?

@dmitriyse
Copy link
Contributor Author

Yes, we can definitely close this PR. It's outdated. We will not support .NetStandard lower than 2.0.

@den-run-ai den-run-ai closed this Aug 10, 2017
@dmitriyse dmitriyse deleted the coreclr branch November 17, 2017 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants