Skip to content
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

NetStandard 2.0 support #136

Closed

Conversation

alex-bogomaz
Copy link
Contributor

Now things around NetCore and NetStandard tooling are stabilized
and we can give a new life to @enricosada 's PR #126.

This PR should resolve #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: dotnet/fsharp#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
Copy link

enricosada commented Oct 6, 2017

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

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
Copy link

pkese commented Oct 23, 2017

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

so what's holding back the release?

@stephen-swensen
Copy link
Contributor

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 stephen-swensen changed the title NetStandard 1.6 support NetStandard 2.0 support Oct 29, 2017
@stephen-swensen
Copy link
Contributor

@alex-bogomaz FYI I created an Unquote issue for the failing tests you discovered: #137 (thanks much for reporting the problem).

@@ -62,6 +62,39 @@ module Internal =
#if PORTABLE
outputReducedExprsMsg outputGenericTestFailedMsg
#else
#if NETSTANDARD2_0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-bogomaz is there are reason for limiting the set of supported frameworks for NetStandard 2.0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason was NetStandard support for each particular framework:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-bogomaz thanks, that makes sense. But I wonder if it matters that Fuchu, Xunit 1, MBUnit, and FSI don't support NetStandard since we are loading assemblies dynamically... I forget why we had to limit the PCL build to only the generic test failed function.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephen-swensen Fuchu is replaced by Expecto afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. maybe we should not limit.
i'll test following scenario and get back with results:

  • use dot net sdk 2.0
  • create unit-test project, target net framework 461
  • reference netstandard 2.0 unquote
  • reference full-framework Fuchu

@stephen-swensen
Copy link
Contributor

@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
Copy link
Contributor Author

  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
Copy link
Contributor

@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: #98 ... I imagine it is a fairly niche use-case: "It is required for them to pass AppDomain boundaries".

@enricosada
Copy link

enricosada commented Nov 2, 2017 via email

@alex-bogomaz
Copy link
Contributor Author

hi, so we are keeping NetStandard 2.0 - right?

@stephen-swensen
Copy link
Contributor

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

@stephen-swensen
Copy link
Contributor

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!

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.

Support .NET Standard 2.0
5 participants