Support .NET Standard 2.0 code generation #3215

dsyme · 2017-06-16T17:37:03Z

See https://github.com/dotnet/cli/issues/6863#issuecomment-309066947

I believe these are the changes that are required to support .NET Standard 2.0 code generation.

The flag --targetprofile:netstandard needs to be provided to instruct the compiler that we are generating code under the assumption that the primary assembly is netstandard.dll . I believe we could infer the --targetprofile flag with a bit more work but it's beyond the scope of this PR, though some other changes I've made have made this easier.

I have removed some old assembly version checking code that hacked away on version numbers to report error messages. These errors aren't particularly useful and with .NET Standard 2.0 using version 2.0.0.0 they would fire in all sorts of wrong ways. We weren't actually testing these in our test suite, and hardcoding this logic in the compiler is not a good idea anymore.

@KevinRansom note the update to the SDK file

dsyme · 2017-06-16T20:01:39Z

I checked the repro in https://github.com/dotnet/cli/issues/6863#issuecomment-309066947 by manually replacing the F# compiler binaries in the directory C:\Program Files\dotnet\sdk\2.0.0-preview2-006470\FSharp and this PR works perfectly:

dsyme · 2017-06-16T20:04:27Z

@cartermp This is a high priority change since F# support in the .NET 2.0 SDK is broken for .NET Standard 2.0 programming without it Currently we only have .NET Standard 1.x creation/consumption/testing in this repro.

I will look at adding .NET Standard 2.0 testing, but the change is very important and I think it should likely be integrated faster.

Do you know how we go about getting this into the .NET SDK? Does it draw from "master" or another branch?

thanks

dsyme · 2017-06-16T20:05:23Z

@enricosada I finally understand what you mean about the dependency basis for .NET Standard 2.0 programming being manifestly much simpler.

Pilchie · 2017-06-16T20:28:32Z

@brettfo and @livarcocc @srivatsn

srivatsn · 2017-06-16T20:53:57Z

src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets

@@ -37,7 +37,8 @@ WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and

  <PropertyGroup>
    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' " >mscorlib</TargetProfile>
-    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' " >netcore</TargetProfile>
+    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND ($(TargetFramework.StartsWith(netcoreapp1.)) OR $(TargetFramework.StartsWith(netstandard1.)))" >netcore</TargetProfile>


The TargetFramework property is an alias. One can have a project where TargetFramework=Foo but map it to some real targetframework (see dotnet/project-system#1938 (comment) for an example of this). You could instead check that TargetFrameworkIdenitifier == .NETStandard.NETCore and TargetFrameworkVersion >= v2.0

I copied this from further down in the existing file. So I should change all occurrences?

I just looked at cases down the line and yes I think those should be modified too. TargetFramework is not even canonical - net46 and net4.6 both map to the same framework. So some of the checks below will be broken even for the regular cases. In general, we should avoid interpreting the targetframework property directly.

Note that if you need to do a bunch of version comparisons, you can do something like this and then msbuild lets you do direct integer comparisons - _TargetFrameworkVersionWithoutV > 2.0

srivatsn · 2017-06-16T20:58:45Z

src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets

@@ -37,7 +37,8 @@ WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and

  <PropertyGroup>
    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' " >mscorlib</TargetProfile>
-    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' " >netcore</TargetProfile>
+    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND ($(TargetFramework.StartsWith(netcoreapp1.)) OR $(TargetFramework.StartsWith(netstandard1.)))" >netcore</TargetProfile>
+    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND !$(TargetFramework.StartsWith(netcoreapp1.)) AND !$(TargetFramework.StartsWith(netstandard1.))" >netstandard</TargetProfile>


Also do you want the TargetProfile to be netstandard for netcoreapp2.0? I assume TargetProfile controls what core assembly is used? If so, the core assembly for netcoreapp2.0 is still System.Runtime.dll (netstandard.dll will simply type forward to System.Runtime in the reference assemblies set for netcoreapp2.0

the core assembly for netcoreapp2.0 is still System.Runtime.dll

To double check - when you compile a netcoreapp2.0, you reference a System.Runtime.dll which contains an actual type definition for System.Object, not a forwarder? Yes, then in that case we want --targetprofile:netcore. Thanks!

We should really get rid of the targetprofile flag and infer it (we are much closer to doing that after this PR)

Yes System.Runtime has the actual definition for System.Object in netcoreapp2.0 refs

dsyme · 2017-06-16T21:44:04Z

@srivatsn Updated. Will do a manual test in a bit.

srivatsn · 2017-06-16T21:49:33Z

src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets

-    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND ($(TargetFramework.StartsWith(netcoreapp1.)) OR $(TargetFramework.StartsWith(netstandard1.)))" >netcore</TargetProfile>
-    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND !$(TargetFramework.StartsWith(netcoreapp1.)) AND !$(TargetFramework.StartsWith(netstandard1.))" >netstandard</TargetProfile>
+    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' " >netcore</TargetProfile>
+    <TargetProfile Condition=" '$(TargetFrameworkIdentifier)' == '.NETStandard' AND '$(TargetFrameworkVersion)' >= 'v2.0'" >netstandard</TargetProfile>


I don't think this will work - msbuild's >= only works if the string is an integer so you need to strip off the v from the version before doing the comparsion.

@srivatsn I switched to CompareTo on strings

srivatsn · 2017-06-16T22:45:49Z

src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets

@@ -53,7 +53,7 @@ WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and
  </PropertyGroup>

  <PropertyGroup Condition=" '$(DisableImplicitSystemValueTupleReference)' != 'true' ">
-    <_FrameworkNeedsValueTupleReference Condition=" $(TargetFramework.StartsWith(netcoreapp1.)) or $(TargetFramework.StartsWith(netstandard1.)) ">true</_FrameworkNeedsValueTupleReference>
+    <_FrameworkNeedsValueTupleReference Condition=" ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETCoreApp') and $(TargetFrameworkVersion.CompareTo(&quot;v2.0&quot;)) &lt; 0 ">true</_FrameworkNeedsValueTupleReference>
    <_FrameworkNeedsValueTupleReference Condition=" '$(TargetFramework)' == 'net40' or '$(TargetFramework)' == 'net45' or '$(TargetFramework)' == 'net46' or '$(TargetFramework)' == 'net461' or '$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net47' ">true</_FrameworkNeedsValueTupleReference>


Can you please change this line as well - It sounds like basically anything higher than 4.0

dsyme · 2017-06-16T22:48:23Z

@srivatsn I updated the code to use _TargetFrameworkVersionWithoutV, could you double check again? thanks

Can you please change this line as well - It sounds like basically anything higher than 4.0

I think it is anything up to 4.7 but not beyond. System.ValueTuple will be in the .NET framework after that. But we should double check while we're here

enricosada · 2017-06-16T22:57:28Z

@srivatsn @dsyme i think @KevinRansom said in #2983 (comment) we need System.ValueTuple package also in 4.7

.NET 4.7 should need the System.ValueTuple package. The BCL team 'forgot' to add a type forward in the ref assembly for the profile. 4.7.1 should be correct.

dsyme · 2017-06-16T23:32:20Z

OK, this is ready, assuming it stays green. I've tested it manually with

build coreclr
copy /y C:\GitHub\dsyme\visualfsharp\release\coreclr\bin\*  "C:\Program Files\dotnet\sdk\2.0.0-preview2-006470\FSharp" 
dotnet clean
dotnet build -v d > out
dotnet run --project tester\tester.fsproj

on the .NET Standard 2.0 and .NET Core 2.0 project in the repro in https://github.com/dotnet/cli/issues/6863#issuecomment-309066947

I don't believe we can add automated testing until @KevinRansom is back online and helps bring in the .NET Core 2.0 runtime and .NET Standard 2.0 libraries. It depends on what F# project compilation testing is already in place for the .NET SDK build and test.

enricosada · 2017-06-17T00:05:19Z

@dsyme about testing i'll try to send here a PR tomorrow.
anyway an example of install the cli (win+unix) + run tests on sdk 2.0 is https://github.com/fsharp/FsAutoComplete/blob/master/test/FsAutoComplete.IntegrationTests/DotNetSdk2.0/Runner.fsx

I also sent dotnet/cli#6914 to cli, with same repro of @ncave of https://github.com/dotnet/cli/issues/6912

ncave · 2017-06-17T00:46:58Z

@dsyme @enricosada I can confirm this PR fixes dotnet/cli#6912. Thanks @dsyme!

KevinRansom · 2017-06-17T15:29:35Z

@dsyme this PR may be quite a bit bigger than is necessary. Some of the changes here I have in a separate PR related to getting FSI working on FSI.

Anyway I am working on this, I don't think we need a netstandard option. I expect that we will deprecate the netcore too fairly quickly, now that we no longer build portable libraries.

FSC works fine on netstandard 2.0 too if you do what we do for dotnetcli. And yes it is supported, C# uses the same trick and is likely to continue using for the forseeable future. Since the fsharp.net sdk redeploys the compiler in the FSharp.Compiler.Tools nuget package, just replace the two .json files there to unblock it.

FSI does require a couple of changes to work on dotnet 2.0, because apparently we rely on a bug that was fixed, that causes us to stack overflow.

The thing stopping me checking in my fixes are that I am struggling to rework the testing, and other stuff that keeps cropping up.

Kevin

KevinRansom · 2017-06-17T17:08:21Z

@dsyme ... Hmmm you merged it?

dsyme · 2017-06-19T09:46:20Z

@KevinRansom AFAICS it's the right change now that "netstandard.dll" replaces "mscorlib.dll" and "System.Runtime.dll" when generating .NET Standard 2.0 libraries. At least I don't see anything else we can do.

I do agree that wee should be able to get rid of the targetprofile flag altogether.

@dsyme this PR may be quite a bit bigger than is necessary.

The rest of the PR is just removing very crazy old checks about mscorlib versions being less than "4" - they are dangerous cruft now that the "netstandard" library uses version number 2.0.0.0

Let me know if you have trouble integrating.

dsyme · 2017-06-19T09:46:35Z

@KevinRansom give me a ping later on if you want to discuss

msftclas added the cla-not-required label Jun 16, 2017

dsyme changed the title ~~[WIP] support .NET Standard 2.0 code generation~~ Support .NET Standard 2.0 code generation Jun 16, 2017

srivatsn reviewed Jun 16, 2017

View reviewed changes

support .NET Standard 2.0 code generation

6193c2f

dsyme force-pushed the ns1 branch from 0e01a4c to 6193c2f Compare June 16, 2017 23:30

dsyme mentioned this pull request Jun 16, 2017

Increase F# test coverage dotnet/sdk#1334

Closed

dsyme merged commit a490bbe into dotnet:master Jun 17, 2017

dsyme mentioned this pull request Jun 27, 2017

[WIP] Switch to .NET Standard 2.0 migueldeicaza/TensorFlowSharp#69

Closed

enricosada mentioned this pull request Jul 4, 2017

lib targeting netstandard2.0 parse ok but intellisense fails ionide/FsAutoComplete#180

Closed

dsyme mentioned this pull request Jul 5, 2017

[Obsolete] How to enable type providers with new-style .NET SDK project files, dotnet build, .NET Standard and .NET Core programming #3303

Closed

Support .NET Standard 2.0 code generation #3215

Support .NET Standard 2.0 code generation #3215

Uh oh!

Conversation

dsyme commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Jun 16, 2017

Uh oh!

dsyme commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Jun 16, 2017

Uh oh!

Pilchie commented Jun 16, 2017

Uh oh!

srivatsn Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dsyme commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dsyme commented Jun 16, 2017

Uh oh!

enricosada commented Jun 16, 2017

Uh oh!

dsyme commented Jun 16, 2017

Uh oh!

enricosada commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncave commented Jun 17, 2017

Uh oh!

KevinRansom commented Jun 17, 2017

Uh oh!

KevinRansom commented Jun 17, 2017

Uh oh!

dsyme commented Jun 19, 2017

Uh oh!

dsyme commented Jun 19, 2017

Uh oh!

Uh oh!

dsyme commented Jun 16, 2017 •

edited

Loading

dsyme commented Jun 16, 2017 •

edited

Loading

srivatsn Jun 16, 2017 •

edited

Loading

dsyme commented Jun 16, 2017 •

edited

Loading

enricosada commented Jun 17, 2017 •

edited

Loading