NetStandard 2.0 support

alex-bogomaz commented 6 years ago

Now things around NetCore and NetStandard tooling are stabilized and we can give a new life to @enricosada 's PR https://github.com/SwensenSoftware/unquote/pull/126.

This PR should resolve https://github.com/SwensenSoftware/unquote/issues/130. Please review.

Building

To build Netstandard 1.6 Unquote you need to have NetCore SDK 2.0:

>dotnet --info
.NET Command Line Tools (2.0.0-preview2-006497)

Now build.bat also builds Unquote.NetStandard\Unquote.NetStandard.fsproj project and merges NetStandard 1.6 dll into existing NuGet package.

Tests

To run NetCore tests execute following:

cd UnquoteTests.NetCore
dotnet restore
dotnet test

Few notes:

CustomExceptionSerializationTests.fs tests are excluded: because of limited serialization support in NetStandard 1.6. The question here - should we have support of NetStandard 2.0 for Unquote and what are use-cases for such exception serialization?
- Following test is skipped:
```
let ``ToUIntPtr primitive`` () =
testEvalCheckedOverflow <@ Checked.unativeint UInt64.MaxValue @>
```
  Not sure why this operation is not failing under NetCore.
Two following DecompilationTests are failing:
1. re-sugar partial application with first, single tuple arg applied
2. re-sugar partial application with first single arg applied and second tuple arg applied with errors like:
```
"ftupled 1 2" = "ftupled (1, 2)" 
false
```
  but this is not something NetCore-related, but rather difference between F#4.0 and 4.1. There are exactly same exceptions when compiling Unquote using VS 2017. In VS 2015 those 2 tests are green.

I've asked the question here: https://github.com/Microsoft/visualfsharp/issues/3698 but still have trouble understanding how to fix this issue in Unquote.

Testing frameworks support.

Assertions.fs includes Unquote NetStandard 1.6 support for Expecto, Xunit2 and NUnit3 frameworks. To test support execute following:

Xunit2:

cd VerifyXunit2Support
dotnet restore VerifyXunit2Support.netcoreapp.fsproj
dotnet test VerifyXunit2Support.netcoreapp.fsproj

NUnit3:

cd VerifyNunit3Support
dotnet restore VerifyNUnit3Support.netcoreapp.fsproj
dotnet test VerifyNUnit3Support.netcoreapp.fsproj

Expecto:

cd VerifyExpectoSupport
dotnet restore VerifyExpectoSupport.netcoreapp.fsproj
dotnet run -p VerifyExpectoSupport.netcoreapp.fsproj

enricosada commented 6 years ago

@alex-bogomaz some notes:

use .net core sdk 2.0.0 (RTM)

dont use dotnet 2.0.0-preview2-006497, this is an old prerelease version. use 2.0.0 RTM

target .net standard 2.0, not 1.6.

and no need to target .net standard 1.6. go directly to netstandard2.0 is a lot better.. and netcoreapp2.0 if needed

use a sln file

as a note. if you want to can create a .sln file

dotnet new sln -n Unquote.netstandard`
dotnet add Unquote.netstandard.sln reference Unquote.NetStandard\Unquote.NetStandard.fsproj

etc.

so you can restore/build it at once

dotnet restore Unquote.netstandard.sln
dotnet build Unquote.netstandard.sln

this is also nicely supported in VSCODE 😄 and make easy to switch netcore/net

refresh template of test

is a good idea to refresh the templates of tests.

you do in a temp dir:

dotnet new xunit -lang F#

to generate an new xunit test project, and check the PackageReference versions with existing. same for nunit

move test project in distinct directories

the project name can be the same. but having multiple fsproj/csproj in same directory has issues, and should be avoided if possible

tomasaschan commented 6 years ago

Just want to put forth that I'm really grateful for someone tackling this. I'm very much looking forward to a NETStandard build of this library - keep it coming! 🎉

pkese commented 6 years ago

Pull request is available,
code review issues have been addressed,

so what's holding back the release?

stephen-swensen commented 6 years ago

Hey all - thanks for effort on this and sorry for slow response. I'll move towards getting this into master and released in the next few days.

My current thinking is that we'll do a major version bump here (Unquote 4.x) and drop support for .NET 4.0 and PCL 256 profiles.

stephen-swensen commented 6 years ago

@alex-bogomaz FYI I created an Unquote issue for the failing tests you discovered: https://github.com/SwensenSoftware/unquote/issues/137 (thanks much for reporting the problem).

stephen-swensen commented 6 years ago

@enricosada @alex-bogomaz question: are there any practical downsides to targeting NetStandard 2.0 instead of NetStandard 1.6? i.e. will we be limiting the platform reach of our NetStandard build?

According to https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-platforms-support

"in general, we recommend you to target the lowest version of .NET Standard possible"
PCL profile 259 (which Unquote currently supports) is compatible with .NET Standard 1.0

alex-bogomaz commented 6 years ago

1) There is no testing frameworks support in PCL Unquote, so we really need Unquote for Standard 1.6 or 2.0 to get nice exceptions in Expecto etc.

2) As for Netstandard 1.6 - just like in PCL case there will be no "serializable" attribute and "serialization constructor" for exceptions like AssertionFailedException. This is the major difference for Unquote compared to Netstandard 2.0

If this is OK - we can target Standard 1.6. Question here: what is the reason of having such constructors for Unquote exceptions?

Btw - i still need to check if we should limit number of supported testing frameworks for Netstandard...

stephen-swensen commented 6 years ago

@alex-bogomaz I think it is fine not to have custom exception serialization if that is the only limitation and NetStandard 1.6 opens up wider platform support. The request for supporting serialization in custom exceptions goes back to 2012: https://github.com/SwensenSoftware/unquote/issues/98 ... I imagine it is a fairly niche use-case: "It is required for them to pass AppDomain boundaries".

enricosada commented 6 years ago

Just ignore net standard1.6, target only 2.0 The 2.0 is the only one who matter. The 1.6 doesn't add more (not obsolete) net profiles To justify it, and tooling isn't there for it. The new lts is soon the 2.0

From mobile

-- Enrico Sada @enricosada

From: Stephen Swensen notifications@github.com Sent: Wednesday, November 1, 2017 9:12:45 PM To: SwensenSoftware/unquote Cc: Enrico Sada; Mention Subject: Re: [SwensenSoftware/unquote] NetStandard 2.0 support (#136)

@alex-bogomazhttps://github.com/alex-bogomaz I think it is fine not to have custom exception serialization if that is the only limitation and NetStandard 1.6 opens up wider platform support. The request for supporting serialization in custom exceptions goes back to 2012: #98https://github.com/SwensenSoftware/unquote/issues/98 ... I imagine it is a fairly niche use-case: "It is required for them to pass AppDomain boundaries".

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SwensenSoftware/unquote/pull/136#issuecomment-341227096, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAI_K5VagbMQkLbqTPBkbT3s0yYwTmlaks5syNC9gaJpZM4PwL-g.

alex-bogomaz commented 6 years ago

hi, so we are keeping NetStandard 2.0 - right?

stephen-swensen commented 6 years ago

@alex-bogomaz yeah sounds like that's the way to go

stephen-swensen commented 6 years ago

Closing this pull request: I merged the changes to master from command line. Prepping for release now. Thanks @alex-bogomaz and @enricosada for contributing this work!

SwensenSoftware / unquote