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

Support .NET Core TP loading and netstandard2.0 #1118

Merged
merged 60 commits into from
Apr 4, 2018

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented Jan 2, 2018

[ edited and updated by @dsyme. Joint work by @baronfel and @dsyme ]

This modernizes FSharp.Data by allowing it to be used with both .NET Core and .NET Framework/Mono-based F# tooling, including the F# compiler in the .NET SDK.

With this work we will have FSharp.Data fully usable in both .NET Core and .NET Framework-based F# tooling. It is also backwards compatible for use with previous F# tooling including previous editions of Visual Studio, Rider and Ionide (though if you want PCL support you have to stick to FSharp.Data 2.x)

  • Update all project files to .NET SDK
  • Multi-target FSharp.Data.dll to netstandard2.0; net45
  • Multi-target FSharp.Data.DesignTime.dll to netstandard2.0; net45
  • Rejig the nuget package layout as per FST-1003
  • Remove support for PCLs of FSharp.Data.dll
  • Test on both netcoreapp2.0 and net46 using the .NET SDK
  • Remove use of old version of SourceLink 1.x
  • Bump to version 3.0.0-beta
  • Test on net46 using Mono msbuild toolchain and F# compiler
  • Test on net46 using .NET Framework msbuild toolchain and F# compiler (at the moment all of Linux/OSX/Windows testing is with .NET SDK dotnet toolchain)
  • Set USE_MSBUILD=1 as part of appveyor matrix
  • Run tests when USE_MSBUILD=1 set
  • Opening FSharp.Data.sln and building inside VS2017 just works
  • Disable failing test that requires compiler to run with .NET Framework, unless USE_MSBUILD is set
  • AppVeyor CI passes
  • Document workarounds for missing encodings on .NET core (in RELEASE_NOTES.md)
  • Travis CI passes
  • Opening FSharp.Data.Tests.sln and running tests inside VS2017 just works

To look at later (after this PR I think):

  • The documentation tests only run on Mono but feel flaky. For example, google started blocking some google finance requests when trying to run them on my Windows machine.
  • I disabled the World Bank signature tests on the local machine (they are already disabled on the build server). Updating these should be orthogonal
  • The PR removed the use of paket.references in favour of explicit PackageReference because of Do framework conditional references in paket.references work with .NET SDK-style projects? Paket#3091.

Notes:

  • FSharp.Data multi-targets to net45; netstandard2.0. We could add more, e.g. netstandard1.6, though there's not much need)
  • FSharp.Data.DesignTime multi-targets to net45; netstandard2.0. We don't need to add more since F# tooling always assumes one of these
  • Use .NET SDK version 2.1.100-preview-007363 to test this
  • One fix was needed to the TPSDK, that is incorporated

OLD NOTES

A new fake build task builds the netcore stuff and the nuspec has been updated with the appropriate version constraints.

Sadly it appears we have to maintain this two-solution approach because the PCL versions of FSharp.Core have been dropped as of somewhere around FSharp.Core version 4.1.17 or 18, so we need an old-project-file-style build to handle building PCLs.

Bonus points, and something I tried very hard to do, was get some subset of the tests building on .Net standard 2.0 as well. I ran into problems here because of Nancy dependencies not playing nicely, so perhaps an enhancement to switch the test server over to Suave?

@baronfel
Copy link
Contributor Author

baronfel commented Jan 2, 2018

image

Package structure after this work^

@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2018

@baronfel I'm going to make a push to get FSharp.Data working with the previews of the latest .NET SDKs and able to target netstandard2.0 and netcoreapp2.0. I'll take a look at this PR as part of this

Sadly it appears we have to maintain this two-solution approach because the PCL versions of FSharp.Core have been dropped as of somewhere around FSharp.Core version 4.1.17 or 18, so we need an old-project-file-style build to handle building PCLs.

I think we should just drop the PCLs now and declare a major version update to FSharp.Data. People building PCLs should continue to use the old version.

@baronfel
Copy link
Contributor Author

I'd be in favor of dropping PCL, I just wish we had some data to put behind making that decision. The main use case for PCLs used to be mobile development, right? And those environments now have runtimes that are net standard 2.0 compatible?

@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2018

@baronfel Yes, the Xamarin runtimes all now support .NET Standard 2.0. Support for PCLs is more or less being deprecated across the whole ecosystem.

That said, .NET Standard 2.0 is not the specific nirvana for mobile - for the shared code in mobile apps you actually want multi-targeting, where the shared code targets both MonoDroid (Android) and MonoTouch (iOS)

@ovatsus
Copy link

ovatsus commented Feb 27, 2018

Does this mean that only F# 4.1 will be supported, and support for 3.1 and 4.0 will be dropped?

@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2018

Does this mean that only F# 4.1 will be supported, and support for 3.1 and 4.0 will be dropped?

@ovatsus I don't think it's necessary to drop support for those

Note that it's not about which F# language version but which FSharp.Core versions the TPRTC(s) are compatible with and which F# tooling versions/deployments the TPDTC(s) will load into. It should be possible to maintain compat though perhaps tricky to test

I'm making some adjustments to this PR and I'll draw up a matrix when I submit, thanks.

@dsyme dsyme changed the title [WIP] Add a netstandard2.0 build and update packaging [WIP] Add netstandard2.0, remove PCL support, support FST-1003 TP loading Feb 27, 2018
@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2018

@ovatsus @baronfel I have submitted work directly to this PR - see update of the PR description

@dsyme dsyme changed the title [WIP] Add netstandard2.0, remove PCL support, support FST-1003 TP loading [WIP] Support .NET Core TP loading and netstandard2.0 Feb 28, 2018
@dsyme dsyme changed the title [WIP] Support .NET Core TP loading and netstandard2.0 Support .NET Core TP loading and netstandard2.0 Feb 28, 2018
@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

AppVeyor is now passing, with both .NET SDK configuration (the default) and USE_MSBUILD=1

Travis is having an issue because the .NET SDK was not being installed, seeing if that's fixed now

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@baronfel The problem with FSharp.Data.Tests.sln is annoying, it looks like the VS tooling doesn't like references to out-of-solution projects for new-style project files.

I'm not sure what the right solution to this is. One would be not to have `to require a local build of the packages and use a PackageReference. But this feels wrong.

I've sent this query to @TIHan

Hey Will,

We have an issue in FSharp.Data involving a ProjectReference to a project not in the solution, e.g.

FSharp.Data.sln  
           FSharp.Data.fsproj
           FSharp.Data.DesignTime.fsproj

FSharp.Data.Tests.sln  
           FSharp.Data.DesignTime.Tests.fsproj --> contains project reference to FSharp.Data.fsproj and FSharp.Data.DesignTime.fsproj

Things build OK from the command line with both “msbuild.exe” and “dotnet build” but when we open FSharp.Data.Tests.sln (or FSharp.Data.DesignTime.Tests.fsproj) using 15.6.4 we get this:
image

What’s the expected behaviour when opening projects that contain references to other projects that are not in the solution? Is there are workaround for this? We can’t easily put everything in one solution because the type provider design-time component gets locked when in use from the test projects

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

Apart from the issue of opening FSharp.Data.Tests.sln and getting Travis green I think we're clear of known issues now (though we should double check everything! :) )

@ovatsus
Copy link

ovatsus commented Mar 28, 2018

Yes, that should be it

@baronfel
Copy link
Contributor Author

I have similar IDE issues in VS Code when launching the test solution, at least until I debug build FSharp.Data.DesignTime/FSharp.Data. The error explicitly asks for debug binaries to be built, and once that's done Ionide begins working again.

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@baronfel @baronfel I pushed a fix so that using both FSharp.Data.sln and FSharp.Data.Tests.sln work for build/edit. I'm pretty sure it's a sound approach - we use Reference nodes and layout the files under bin just like the nuget package.

@ovatsus
Copy link

ovatsus commented Mar 29, 2018

If I switch to net461 in the tests I get squiggly lines everywhere:

image

@ovatsus
Copy link

ovatsus commented Apr 2, 2018

To be clear, I don't think that's a deal breaker, as soon as CI is passing I think this should be good to go.

@baronfel , @dsyme, are any of you looking at the mono test failure?

@baronfel
Copy link
Contributor Author

baronfel commented Apr 2, 2018

The OSX, latest mono, dotnet-cli failures seem to be transient and due to invalid json, I'm not sure they represent an actual problem in the code.

The Linux, weekly mono, msbuild failures are more concerning. I can't replicate them locally on OSX/mono 5.10 as of yet.

@dsyme
Copy link
Contributor

dsyme commented Apr 4, 2018

These are the failures on Linux Mono weekly:

Error : FSharp.Data.Tests.Http.Web request's timeout is used
System.NullReferenceException : Object reference not set to an instance of an object
at FSharp.Data.Tests.Http.Web request's timeout is used () [0x00026] in <5abc27ca49fdba00a7450383ca27bc5a>:0 
  at (wrapper managed-to-native) 

Failed : FSharp.Data.Tests.HttpIntegrationTests.all details of the response should be available
 Expected: <Text "Some JSON or whatever">
 But was:  <Text "">

@dsyme
Copy link
Contributor

dsyme commented Apr 4, 2018

@ovatsus @baronfel I don't think the two failures in Mono weekly should stop us integrating. They may be known bugs in Mono that are fixed before release, or we may need to record bugs against Mono. I'll mark the weekly as a configuration we can ignore errors on (I don't like doing that of course)

@dsyme
Copy link
Contributor

dsyme commented Apr 4, 2018

We now only have these failures on Mono+latest+OSX+USE_MSBUILD.

It's very odd that this configuration specifically would fail when other very similar ones are passing e.g.
Mono+latest+Linux+USE_MSBUILD
Mono+5.8.0+OSX+USE_MSBUILD

both pass!? So which variable is to blame!? Things like NameResolutionFailure tell me it's a configuration thing.

1) Error : FSharp.Data.Tests.Http.Don't throw exceptions on http error
System.Net.WebException : Error: NameResolutionFailure
2) Error : FSharp.Data.Tests.Http.Setting timeout in customizeHttpRequest overrides timeout argument
System.Net.WebException : Error: NameResolutionFailure
3) Failed : FSharp.Data.Tests.Http.testFormDataBodySize(4000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
4) Failed : FSharp.Data.Tests.Http.testFormDataBodySize(20000)
  Expected: No Exception to be thrown
5) Failed : FSharp.Data.Tests.Http.testFormDataBodySize(40000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
6) Failed : FSharp.Data.Tests.Http.testFormDataBodySize(100000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
7) Failed : FSharp.Data.Tests.Http.testFormDataBodySize(200000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
8) Failed : FSharp.Data.Tests.Http.testMultipartFormDataBodySize(4000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
9) Failed : FSharp.Data.Tests.Http.testMultipartFormDataBodySize(20000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
10) Failed : FSharp.Data.Tests.Http.testMultipartFormDataBodySize(40000)
  Expected: No Exception to be thrown
  But was:  <System.AggregateException: One or more errors occurred. ---> System.Net.WebException: Error: NameResolutionFailure
11) Failed : FSharp.Data.Tests.Http.testMultipartFormDataBodySize(100000)
  Expected: No Exception to be thrown
12) Failed : FSharp.Data.Tests.Http.testMultipartFormDataBodySize(200000)
  Expected: No Exception to be thrown
13) Error : FSharp.Data.Tests.Http.Timeout argument is used
System.NullReferenceException : Object reference not set to an instance of an object
14) Error : FSharp.Data.Tests.Http.Two custom header with different names don't throw an exception
System.Net.WebException : Error: NameResolutionFailure
15) Error : FSharp.Data.Tests.Http.Web request's timeout is used
System.NullReferenceException : Object reference not set to an instance of an object

@baronfel
Copy link
Contributor Author

baronfel commented Apr 4, 2018

I agree with that route. Ideally I'd like to wrap those tests that use the network stack in a retry mechanism, specifically because we've had so many repeated problems with them.

@ovatsus ovatsus merged commit 6171a8b into fsprojects:master Apr 4, 2018
@ovatsus
Copy link

ovatsus commented Apr 4, 2018

Weird, build release fails with this error:

Running build failed.
Error:
System.Exception: Start of process fsi.exe failed. The system cannot find the file specified
   at Fake.ProcessHelper.ExecProcessWithLambdas@103-16.Invoke(String message) in D:\code\fake\src\app\FakeLib\ProcessHelper.fs:line 103
   at Fake.ProcessHelper.ExecProcessWithLambdas(FSharpFunc`2 configProcessStartInfoF, TimeSpan timeOut, Boolean silent, FSharpFunc`2 errorF, FSharpFunc`2 messageF) in D:\code\fake\src\app\FakeLib\ProcessHelper.fs:line 103
   at Fake.FSIHelper.executeFSIWithArgs(String workingDirectory, String script, FSharpList`1 extraFsiArgs, IEnumerable`1 env) in D:\code\fake\src\app\FakeLib\FSIHelper.fs:line 186
   at FSI_0005.Build.clo@195-33.Invoke(Unit unitVar0)
   at Fake.TargetHelper.runSingleTarget(TargetTemplate`1 target) in D:\code\fake\src\app\FakeLib\TargetHelper.fs:line 626

But if I manually run build.fsx code inside VS it seems to work fine

@ovatsus
Copy link

ovatsus commented Apr 4, 2018

Published 3.0.0-beta on Nuget

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.

7 participants