I don't really know what the right thing to do here is. The TargetFrameworkVersion is the target for which we are generating code, and is independent of whether you want to run the F# compiler using .NET Core or .NET Framework. The way you run the compiler is just orthogonal, though 99% of the time TargetFrameworkVersion is a fine guess.

I suppose it just depends whether you have .NET Core or .NET Framework available on your machine. I don't know how to determine that. This is why Roslyn ended up with two packages, just pushing the problem up another level. The world keeps being bifurcated. It's very difficult to stop but we need to try

@enricosada Do you have any ideas here? I like the idea of a unified package but I don't see any good guidance on working out which is the preferred compiler toolchain

@dsyme is right, one thing is target fw, other is current runtime of msbuild.

we discussed a bit that in dotnet/netcorecli-fsc#75 for another stuff.

you should check $(MSBuildRuntimeType) property (with null check because probably doesnt exists in msbuild 14)

About choose toolchain (use fsc full, fsc netcore, fsc full on mono) in same fsproj.
That is something i want to support, and sort of work already, but need to cleanup (FscToolPath_netcoreapp1_0 is orrible, i need to change that).

Atm if you run with <DontRunFscWithDotnet>true</DontRunFscWithDotnet> (ref FSharp.NET.Core.Sdk.targets#L158 will run fsc.exe, but i want to cleanup, is just an hack atm, to use TP in new fsproj and check both fsc behaviour (for my development).

Idea is instead of use orrible properties like FscToolPath_netcoreapp1_0 (unused atm), to add item metadata about avaiable fsc

<AvaiabileFscTool Include="$(MSBuildThisFileDirectory)netcoreapp1.0/fsc.dll">
   <Runtime>netcoreapp1.0</Runtime>
   <RuntimeType>Core</RuntimeType> <!-- so i know if i need to prefix `dotnet` -->
</AvaiabileFscTool>
<AvaiabileFscTool Include="$(MSBuildThisFileDirectory)/net45/fsc.exe">
   <Runtime>netcoreapp1.0</Runtime>
   <RuntimeType>Full</RuntimeType> <!-- no prefix, or `mono` if not windows -->
</AvaiabileFscTool>

in the fsproj, user can choose to add <FscToolPreferredRuntime>netcoreapp1.0</FscToolPreferredRuntime> and the target file will filter AvaiabileFscTool with that

that's the idea for fsc.

So on msbuild.exe (full) will use full fsc, on netcore the netcoreapp, and on mono the full version (run by mono)

Obv a good FscToolPath is something nice to have for current sdk, because that' how things work now.

@dsyme obv the part about AvaiabileFscTool is just an idea, to support and use multiple compiler in same package easy. but i want to discuss a bit before. i added the unused properties FscToolPath_netcoreapp1_0 just in case i needed it (and was useful in some diagnostics of bugs with users already)

Ah, I didn't realise at the start that the distinction is for fsc runtime, not code generation target. Now things make a bit more sense.

I am currently rerunning the VS Setup so that I can also test this with net core.

@0x53A like that seems ok. should work for your use case, i think the old target doesnt use FscTaskAssemblyPath but hardcode the one install with VS, so i ok.

@dsyme i need to try this PR locally before merge.
i want to test with netcore/full/mono msbuild.
i think is better to add some integration test in this repo too, to check that package.
I'll send a PR stealing from dotnet/fsharp#2250 who already included that, sigh 😢 .

@enricosada Thank you for your help!

One other issue is the Microsoft.FSharp.Targets. Roslyn overwrites their target files to include the one from the nuget package: https://github.com/dotnet/roslyn/blob/master/build/NuGetAdditionalFiles/Microsoft.Net.Compilers.props#L29

While our nuget also includes the Microsoft.FSharp.Targets, it isn't really possible to overwrite it due to the way the defaul fsproj is structured:

When installing the nuget, the props file is included at the top, but the current fsproj include it like this:

  <Choose>
    <When Condition="'$(VisualStudioVersion)' == '11.0'">
      <PropertyGroup Condition="Exists('$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets')">
        <FSharpTargetsPath>$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>
      </PropertyGroup>
    </When>
    <Otherwise>
      <PropertyGroup Condition="Exists('$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)\FSharp\Microsoft.FSharp.Targets')">
        <FSharpTargetsPath>$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)\FSharp\Microsoft.FSharp.Targets</FSharpTargetsPath>
      </PropertyGroup>
    </Otherwise>
  </Choose>
  <Import Project="$(FSharpTargetsPath)" />

So without manually modifying the fsproj, there is no way to override the .targets from this nuget.

This is an issue for example if there is a new compiler parameter.

This is why Roslyn ended up with two packages, just pushing the problem up another level. The world keeps being bifurcated. It's very difficult to stop but we need to try

Roslyn bifurcated afaik because they bundle the csc inside two different stuff (dotnetcli and VS), so was easier just use two packages :D

@0x53A

So without manually modifying the fsproj, there is no way to override the .targets from this nuget.

I don't think there's any way to fix it - the user just has to edit the project file by hand, don't they?

However I would love if someone would go fix this stuff at the root core of the problem - i.e. the templates that are included in the Visual F# Tools - and just make sure that the targets path is at least defined by a variable (or some other fundamental fix). The fact that so many F# project files unavoidably rely on a machine-path installed set of F# targets is really storing up trouble for the long term

@dsyme The easiest way would be to only set the path inside the fsproj if it is not already set.
Changing that in the template shouldn't be hard, but that would obviously only apply to new projects.

Do you have a hook inside VS where such a transformation could be applied on opening the project?

For example simply changing to this:

...
        <FSharpTargetsPath Condition="'$(FSharpTargetsPath)' == ''>$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>
...
        <FSharpTargetsPath Condition="'$(FSharpTargetsPath)' == ''>$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)\FSharp\Microsoft.FSharp.Targets</FSharpTargetsPath>
...
  <Import Project="$(FSharpTargetsPath)" />

so as not to override an existing FSharpTargetsPath setting

@dsyme i dont think is possibile to fix that with old fsprj, the whole sdk was using hardcoded paths or relative to sdk/extensions dir. Is risky, for what? you cannot change old templates.. Yes you can change the FSharpTargetsPath but that target must be really ok, otherwise ide will not work.

the new fsproj fix that. there is no need anymore to worry about.
target file is inside FSharp.NET.Sdk package, so build are deterministic. All target are like that from packages like Microsoft.NET.Sdk.

@enricosada so I should NOT try to set the targets file in this nuget? Then I will revert 0c70c11

the new fsproj fix that. there is no need anymore to worry about.

That's fine once we update the Visual Studio .NET Framework templates to use the new .fsproj. (However Microsoft might not do that for .NET Framework tempaltes because they may want to maintain VS2017 --> VS2015 compat, so projects created with VS2017 can be edited by teams using VS2015 as long as they don't use F# 4.1 features - I'm not sure).

@dsyme i dont think is possibile to fix that with old fsprj, the whole sdk was using hardcoded paths or relative to sdk/extensions dir. Is risky, for what? you cannot change old templates..

Yes, agreed, though editing the templates to check if FSharpTargetsPath is already set seems harmless

@0x53A I think setting FSharpTargetsPath is fine, it will just be ignored (overridden) by nearly all existing project files that this package gets integrated into.

Yes, agreed, though editing the templates to check if FSharpTargetsPath is already set seems harmless

Ah yes, chaning VS2017 old fsproj templates, yes :D

@0x53A I think setting FSharpTargetsPath is fine...

@enricosada Do you agree it is OK to set this? Even if it is ignored/overridden? Seems ok to me

think setting FSharpTargetsPath is fine, it will just be ignored (overridden) by nearly all existing project files that this package gets integrated into

Yes, sry, i checked.
i use LanguageTargets and FSharpLanguageTargets in
https://github.com/dotnet/netcorecli-fsc/blob/master/src/FSharp.Sdk/Sdk/Sdk.targets#L21

i remembered things incorrectly

@enricosada Ah I see.

But regardless should't the setting of FSharpTargetsPath be conditional on the .NET Framework build/compile toolchain, i.e. only set it when when this is false?

Condition="'$(MSBuildRuntimeType)' == 'Core'">

Do we expect to be able to process existing F# projects with the .NET Core build toolchain?

Wouldn't setting FSharpTargetsPath be conditional on the TargetFramework, not on the MSBuildRuntimeType?

Wouldn't setting FSharpTargetsPath be conditional on the TargetFramework, not on the MSBuildRuntimeType

no, becasue new fsproj can build all frameworks, you cannot rely on TargetFramework, example `net451.

Let me see if i can find a msbuild property about new sdk vs old

Let me see if i can find a msbuild property about new sdk vs old

Yes that's what we need.

Ah yeah...

So we have:

old SDK vs new SDK
TargetFramework=Full vs TargetFramework=NetCore (and maybe mono, but I think that is the same as Full?)
MSBuildRuntimeType Full vs Core

old SDK vs new SDK

yes.

TargetFramework

that's the usual (full/pcl), but now there is also netstandard1.6, or netcoreapp1.0 for example

MSBuildRuntimeType Full vs Core

yes, because exists msbuild.exe and dotnet msbuild (the netcoreapp version of msbuild console app)

So, i have better memory than expected.
pratically the package for C# tried to embedded the old target time ago afaik.
i copy/pasted same c# workflow time ago, but not implmented anything for old targets (because is not my area)
https://github.com/dotnet/netcorecli-fsc/blob/master/src/FSharp.NET.Sdk/build/FSharp.NET.Current.Sdk.targets#L37-L52

And yes, i use FSharpTargetPath for new sdk in https://github.com/dotnet/netcorecli-fsc/blob/27791ecbcea992146f1dd652b3e83da03d215dba/src/FSharp.NET.Sdk/build/FSharp.NET.Current.Sdk.targets#L55

But checking new sdk 1.0.1, i see no more that, just targets for new code. i think they just abandoned the idea maybe. I chcked dotnet/sdk but i lost atm :D

@dsyme @0x53A maybe we should do that in another PR?

@dsplaisted @eerhardt there is a way to know if the target is execute in new sdk (with Microsoft.NET.Sdk) or old sdk (the targets file like VS <= 2015)?

@enricosada So you're saying we should not set FSharpTargetsPath at all in this PR, until we determine if there's a way to know if the targets are executed in new SDK (i.e. with Microsoft.NET.Sdk) ? (and we would only set it if it's false)

Isn't the targets file coupled to the actual version of fsc used?
So wouldn't you also want to override the targets file in case of the new sdk?

Isn't the targets file coupled to the actual version of fsc used?

is tied more to fsc task used and sdk used. fsc args are stable, and retro compatible afaik.
but if fsc msbuild task pass newer arguments, fsc will fail, so i was wrong, is tied

So you're saying we should not set FSharpTargetsPath at all in this PR, until we determine if there's a way to know if the targets are executed in new SDK (i.e. with Microsoft.NET.Sdk) ? (and we would only set it if it's false)

that's better, or give me some time to test, i dont want to break things 😄

Well, as long as the SDK is also on nuget, it doesn't really matter if the targets file is in the sdk nuget or in the FSharp.Compiler.Tools nuget, since you can just update both. =)

The current problem is that it uses the targets from the installed visual studio.

I have created a new issue for FSharpTargetsPath: #676

@0x53A Could you please also add notes in the README.md about how to add the FSharp.Compiler.Tools to an existing project as part of this PR, mentioning the FSharpTargetsPath issue? Thanks!

I have added a note to fsharp/fsharp about the issue with the targets file, and also added a quick note that the package exists (e.g. for use on a buildserver) to visualfsharp.

@dsplaisted @eerhardt there is a way to know if the target is execute in new sdk (with Microsoft.NET.Sdk) or old sdk (the targets file like VS <= 2015)?

Officially there isn't a way yet. See dotnet/sdk#534

Unofficially, there are ways to accomplish this. One way (again not officially supported) is how the targets in .NET Standard are doing it: https://github.com/dotnet/standard/blob/master/Microsoft.Packaging.Tools/tasks/targets/Microsoft.Packaging.Tools.targets#L11-L12

I assume I should update the readme.md in the visualfsharp repo?

No the one in this repo - they are additive - they haven't yet been merged (some of the last remaining changes)

thx for info @eerhardt !