update F# templates to no longer unconditionally override FSharpTargetsPath. #2575

0x53A · 2017-03-10T17:09:26Z

This allows to set FSharpTargetsPath from outside.

…tsPath. This allows to set FSharpTargetsPath from outside.

msftclas · 2017-03-10T17:09:30Z

@0x53A,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

enricosada · 2017-03-10T17:10:52Z

vsintegration/ProjectTemplates/ConsoleProject/Template/ConsoleApplication.fsproj

@@ -70,12 +70,12 @@
  <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>
+        <FSharpTargetsPath Condition="'$(FSharpTargetsPath)'==''">$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>


as a style note (easier to read), use spaced after and before ", so like

Condition=" '$(FSharpTargetsPath)' == '' ">

It would be fair to say that we don't do that spacing anywhere near consistently. So it's fine as it is. Checkout the line immediately above :-).

I guess the condition could go on the "Choose" (I assume it supported conditions)

@dsyme I have no idea, but Intellisense didn't offer Condition on Choose.

@dsyme I was going to check that ... but it makes sense for it not to be there since Choose is itself a condition.

You could of course put it in a property group with a condition which is probably the right approach here.

@KevinRansom according to IntelliSense, there is not Choose inside PropertyGroup ...

dsyme · 2017-03-10T17:13:05Z

@0x53A Can you also have a think what to do about the portable templates? These

https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/ProjectTemplates/NetCore259Project/Template/PortableLibrary.fsproj#L56

These should probably be changed to define a conditional "FSharpPortableTargetsPath" and check if it's not already set? And your prop file in the FSharp.Compiler.Tools nuget package can set this.

@KevinRansom We know these templates are rapidly becoming legacy, but see the context in the linked issue above - we want to make sure that the FSharp.Compiler.Tools nuget package can be added to an existing project and things will work correctly, at least for as long as these templates continue to exist.

0x53A · 2017-03-10T17:30:17Z

@dsyme I added the same logic to the portables, but I am not sure if that is correct. Because the portable target includes a bunch of other files before including the standard target: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Build/Microsoft.Portable.FSharp.targets

So I think the better solution would be to define two properties, FSharpTargetsPath and PortableFSharpTargetsPath, where the default templates check against FSharpTargetsPath and the portable templates check against PortableFSharpTargetsPath.

KevinRansom · 2017-03-10T17:30:58Z

Well ... it does what says on the tin ... and it allows the TARGETSPATH to be set externally to the project file and with the FSharp.Tools nuget package you need some mechanism to point the project to it's version of the files. It would be fair to say I'm not a fan of that packaging either ... since it misses all of the multi-targeting support, and so is tied to building FSharp 4.1 Desktop apps and libraries.

This change will work fine with portable libraries if VS is installed.

And ... as you say ... everything will change with the next release ... although of course legacy projects like this will still work.

So ... I am Okay with this change.

dsyme · 2017-03-10T17:34:19Z

So I think the better solution would be to define two properties, FSharpTargetsPath and PortableFSharpTargetsPath, where the default templates check against FSharpTargetsPath and the portable templates check against PortableFSharpTargetsPath.

Yes, please do exactly that

KevinRansom

@dsyme @0x53A The change as is should work fine for portables. TargetPath is only used to find the targets files. Why do you think it needs a separate variable?

KevinRansom · 2017-03-10T17:34:38Z

vsintegration/ProjectTemplates/ConsoleProject/Template/ConsoleApplication.fsproj

@@ -70,12 +70,12 @@
  <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>
+        <FSharpTargetsPath Condition="'$(FSharpTargetsPath)'==''">$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>


@dsyme I was going to check that ... but it makes sense for it not to be there since Choose is itself a condition.

You could of course put it in a property group with a condition which is probably the right approach here.

dsyme · 2017-03-10T17:39:18Z

It would be fair to say I'm not a fan of that packaging either ... since it misses all of the multi-targeting support, and so is tied to building FSharp 4.1 Desktop apps and libraries.

Hmmm... @enricosada has integrated the .NET Core compiler into the FSharp.Compiler.Tools package as well, and as far as I understand it is the package referenced by netcorecli-fsc. See these lines in the nuget file and the overall process for building the nuget package. The files that are brought in still ultimately come from Microsoft.FSharp.Compiler.netcore but are effectively repackaged into this package. I think @enricosada did it this way to get all the right SDK bits and pieces - targets etc. - into place, I'm not sure.

So for the purposes of .NET Core right now I think that package is currently the way of distributing the compiler tools to the end user.

I'd be very happy to bring that nuget package specification into this repo so we just have one compiler tools package (as long as we double check that we're also still packaging a "good" f# compiler for use on Mono).

KevinRansom · 2017-03-10T17:41:56Z

Actually, I am not a fan of it for the desktop, for the reasons I gave above. Coreclr packaging is a much different propostion.

dsyme · 2017-03-10T17:45:35Z

Actually, I am not a fan of it for the desktop, for the reasons I gave above. Coreclr packaging is a much different propostion.

ok

dsyme · 2017-03-10T17:49:31Z

@KevinRansom @enricosada Is there any reason we can't move both the spec of the FSharp.Compiler.Tools package and the whole of netcorecli-fsc into this repo? I find it really very confusing to have the .NET Core compiler packaging and delivery spread over three repos. At the moment bits and pieces of all three repos come into play.

Right now we're going to have a hell of a time keeping all this in sync and publishing new editions of the compiler tooling quickly?

dsyme · 2017-03-10T17:56:23Z

Actually, I am not a fan of it for the desktop, ... [since it misses all of the multi-targeting support, and so is tied to building FSharp 4.1 Desktop apps and libraries.]

Hmmm... thinking about this more.... Is there really any reason why the .NET Framework compiler bundled in that package can't be used to target portable, .NET Standard and .NET Core? As long as the reference assemblies are found (either by either Paket or the new MSBuild SDK)?

Or were you referring to some other missing capability? I just want to know the limitations, thanks. My impression is that we're now in a world where all of these are othogonal

.NET Framework fsc.exe v. .NET Core fsc.dll
Old MSBuild SDK (Microsoft.FSharp.targets) v. New MSBuild SDK (FSharp.NET.Sdk)
.NETFramework v. .NETPortable v. .NET Standard v. .NETCoreApp1.0

Are there any combinations of these that we don't expect to work, assuming reference assemblies are found correctly?

enricosada · 2017-03-10T18:05:01Z

@enricosada did it this way to get all the right SDK bits and pieces - targets etc. - into place, I'm not sure

as a note, for new sdk, i dont use the target file from vf# repo. i use FSharp.NET.Core.Sdk.targets, that's where CoreCompile is defined

Is there any reason we can't move both the spec of the FSharp.Compiler.Tools package

for FSharp.Compiler.Tools i dont mind. it's pratically Microsoft.FSharp.Compiler.netcore with some props file to locate fsc.
As a note, i tried initially to do exactly that with my old PR #2250 .
Is the same as having FSharp.Core package here ihmo, why not? i am just merging Microsoft.FSharp.Core.netcore too.
Also for FSharp.Build.dll having that signed from MS is better, because of internal visible stuff.

and the whole of netcorecli-fsc into this repo

Nothing techically, is all about project development.
Atm i can:

move faster fixing bugs and release packages in other repo
for speed of the test suite too (i have far more os under test and atm is fast to generate a package).
i can personally manage issues/tag, manage roadmap for sdk 1.0 rtm https://github.com/dotnet/netcorecli-fsc/projects/2, or releases
i triage issue from using new fsproj from netcore users (or xamarin, or sdk team), if is a compiler issue i reopen here (like today fsc netcore --help shows pdbonly and full. and --debug:__ ignores these options #2565), if is a fsharp.core bug too.

long term is a good idea to have it near FSharp.Core and FSharp.Compiler.Tools (so maybe in fsharp/fsharp too, just compiler+fsharcore+fcs? 😄 ).
But atm is a bit complicated to do so.

KevinRansom · 2017-03-10T18:05:55Z

@dsyme
In order to build fsharp portable libraries you will need the dotnet targeting packs and the ms fsharp compiler installed along with the the extra versions of fsharp.core.dll, if they are installed then there is no need for this package, I assume that one can grab alternate fsharp.core.dlls from a nuget package. So I suppose that a combination of the two packages is fine. However ... the VS multi-targeting dialog doesn't know about any of that stuff so it either won't work or will mess up the project.

But yes paket or nuget + msbuild + dotnet targeting packs + the right project file magic and it will work fine.

enricosada · 2017-03-10T18:07:39Z

Or were you referring to some other missing capability?

ngen is one (and is good speed bump right).
Atm i cannot ngen the fsc embedded in FSharp.Compiler.tools. i can use net45 fsc in new sdk. i can use it on mono too. but no ngen.

enricosada · 2017-03-10T18:10:08Z

@dsyme annother limitation is new sdk cannot built .net framework 3.5 if matter for someone

on mono for net45 you need reference assebmlies, but beacuse are not published as nuget package, you need to override a propertu for find these in mono installation

dsyme · 2017-03-10T18:13:38Z

In order to build fsharp portable libraries you will need the dotnet targeting packs

Yes agreed

and the ms fsharp compiler

(Or the FSharp.Compiler.Tools fsc.exe)

along with the the extra versions of fsharp.core.dll
I assume that one can grab alternate fsharp.core.dlls from a nuget package.

Yes, the FSharp.Core package has the PCL DLLs, so people referencing that just pick them up. Also those DLLs aren't always in the right pace on Mono. It's much easier to use the nuget package. This guidance I wrote on building PCLs and FSharp.Core says "use the FSharp.Core nuget package".

However ... the VS multi-targeting dialog doesn't know about any of that stuff so it either won't work or will mess up the project.

Yes, agreed

enricosada · 2017-03-10T18:18:27Z

use the FSharp.Core nuget package

that should be for everything. Otherwise is possibile to mix ref to gac and ref to nuget package fsharp.core, and is annoying.
With vs2017 ihmo fsharp.core should be from nuget package in templates, because we require System.ValueTuple anyway in a packages.config, and all project use nuget
@KevinRansom @dsyme is not better to change templates from now on and use fsharp.core package as default? for lib and console at least

0x53A · 2017-03-10T18:39:58Z

Sorry, this whole pcl / multi-targeting / ... stuff is a bit out of my league.

I have modified the spacing of the conditions to look like @enricosada proposed, is there anything else I should change?

(Also, +1 to aquiring FSharp.Core from nuget. VS2017 includes a few selected packages, this should include FSharp.Core so that it will also work in an offline scenario)

dsyme · 2017-03-10T18:42:29Z

Sorry, this whole pcl / multi-targeting / ... stuff is a bit out of my league.

Don't worry, it's out of my league too :) It's a really big matrix unfortunately. Hopefully in 6 months a lot of it will really, truly be legacy-only.

dsyme · 2017-03-10T18:46:09Z

@KevinRansom @dsyme is not better to change templates from now on and use fsharp.core package as default? for lib and console at least

Yes. But the blockers are

We should only do this if Microsoft publish and upload the package (or do it with the community)
However the package also contains delay-signed Xamarin-friendly FSharp.Core profile DLLs for the xamarinios etc. profiles, built from http://github.com/fsharp/fsharp. The status of those is a wee bit unclear. We could build those in this repo. But I don't even understand if they will be needed long term. We have to clarify that. Jason Imison and @migueldeicaza are good initial contacts.

KevinRansom · 2017-03-10T18:48:41Z

@enrico Sada<mailto:enrico@sada.io> Changing the templates is fine … we know more changes are coming but until we get to the future … incremental improvements expand what can be done sooner.

KevinRansom · 2017-03-10T19:01:03Z

vsintegration/ProjectTemplates/ConsoleProject/Template/ConsoleApplication.fsproj

@@ -70,12 +70,12 @@
  <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>
+        <FSharpTargetsPath Condition=" '$(FSharpTargetsPath)' == '' ">$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>


Checkout the propertygroup condition above ...
Your code will only happen if ... the correct targets file exists in the regular vs location .... which probably defeats the point of the change.

So I think this section should probably look similar to this ... written in a text editor, completely untested.
The idea, set FSTP to the directory containing the target files.
For portable projects, change the last line to point to the portable target file.

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

Before, FSharpTargetsPath pointed directly to the file. With your change, FSharpTargetsPath would point to the directory. Is that on purpose?

And yes, the two conditions could be unified to a single one combined with and, but if I'm not missing anything that wouldn't actually change the logic? (Except maybe make it easier to read?)

yes ... then you can use the same code for portable. And a single variable to locate the target files.

enricosada · 2017-03-10T19:41:15Z

We should only do this if Microsoft publish and upload the package (or do it with the community)

@dsyme with commmunity is preferred ihmo. But There are lot of examples already in ms, aspnet use newtonsoft.json, xunit, etc. So is not a blocker to use non ms packages in templates in other products

KevinRansom · 2017-03-10T19:55:47Z

@dsyme @enricosada I think it's fine as it is with the community owning this package, it is an enthusiast packaging for enthusiast scenarios.

dsyme · 2017-03-10T21:24:16Z

@dsyme with commmunity is preferred ihmo. But there are lot of examples already in
ms, aspnet use newtonsoft.json, xunit, etc. So is not a blocker to use non ms packages in
templates in other products

You are correct that Newtownsoft JSON and Xunit have already set a precedent.

@dsyme @enricosada I think it's fine as it is with the community owning this package

I would certainly be comfortable with the FSharp.Core package being handled by Microsoft. But perhaps what's most important is that it contains the signed binaries from Microsoft.

KevinRansom · 2017-03-10T19:10:22Z

vsintegration/ProjectTemplates/ConsoleProject/Template/ConsoleApplication.fsproj

@@ -70,12 +70,12 @@
  <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>
+        <FSharpTargetsPath Condition=" '$(FSharpTargetsPath)' == '' ">$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\3.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>


yes ... then you can use the same code for portable. And a single variable to locate the target files.

KevinRansom · 2017-03-10T21:32:11Z

vsintegration/ProjectTemplates/NetCore259Project/Template/PortableLibrary.fsproj

+  <PropertyGroup>
+    <PortableFSharpTargetsPath Condition=" '$(PortableFSharpTargetsPath)' == '' ">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)\FSharp\Microsoft.Portable.FSharp.Targets</PortableFSharpTargetsPath>
+  </PropertyGroup>
+  <Import Project="$(PortableFSharpTargetsPath)" />


Really ... you want to set two paths ... it seems a bit ... redundant.

Still ... I guess it's harmless enough.

As long as there is any way to override these targets files, I do not really care about if there is one variable or two. But FSharpTargetsPath has historically pointed directly to the file, and a few people have already manually changed their project files and hardcoded this. So I think this would cause a bit of confusion, and the whole topic is already complex enough.

So if we want to use a single variable (which I actually also like), then I think we should use a different name, e.g. FSharpTargetsDir, to make clear it points to the directory. @KevinRansom What do you think?

update F# templates to no longer unconditionally override FSharpTarge…

99854e2

…tsPath. This allows to set FSharpTargetsPath from outside.

msftclas added the cla-already-signed label Mar 10, 2017

enricosada reviewed Mar 10, 2017

View reviewed changes

also allow override of FSharpTargetsPath in portable project templates

9803f0c

use PortableFSharpTargetsPath for portable templates

a22ccd2

KevinRansom reviewed Mar 10, 2017

View reviewed changes

updated templates: better spacing

25f58a2

0x53A mentioned this pull request Mar 10, 2017

Investigate if and how FSharp.Compiler.Tools can override FSharpTargetsPath fsharp/fsharp#676

Closed

0x53A mentioned this pull request Mar 10, 2017

Include FSharp.Core in the offline package directory #2577

Closed

KevinRansom requested changes Mar 10, 2017

View reviewed changes

unify conditions

40d3c1f

dsyme approved these changes Mar 10, 2017

View reviewed changes

KevinRansom approved these changes Mar 10, 2017

View reviewed changes

KevinRansom merged commit f5796b4 into dotnet:master Mar 10, 2017

0x53A mentioned this pull request Mar 11, 2017

FSharp.Compiler.Tools: set FSharpTargetsPath when using the old SDK fsharp/fsharp#680

Merged

0x53A mentioned this pull request Apr 3, 2017

update targets in templates #2776

Closed

0x53A deleted the update-templates branch April 16, 2017 14:38

0x53A mentioned this pull request Apr 25, 2017

Automagically fix up choose block when importing FSharp.Compiler.Tools fsprojects/Paket#2280

Closed

update F# templates to no longer unconditionally override FSharpTargetsPath. #2575

update F# templates to no longer unconditionally override FSharpTargetsPath. #2575

Conversation

0x53A commented Mar 10, 2017

msftclas commented Mar 10, 2017

enricosada Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Mar 10, 2017 • edited Loading

0x53A commented Mar 10, 2017

KevinRansom commented Mar 10, 2017 • edited Loading

dsyme commented Mar 10, 2017

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Mar 10, 2017 • edited Loading

KevinRansom commented Mar 10, 2017 via email

dsyme commented Mar 10, 2017

dsyme commented Mar 10, 2017 • edited Loading

dsyme commented Mar 10, 2017

enricosada commented Mar 10, 2017

KevinRansom commented Mar 10, 2017

enricosada commented Mar 10, 2017 • edited Loading

enricosada commented Mar 10, 2017

dsyme commented Mar 10, 2017

enricosada commented Mar 10, 2017 • edited Loading

0x53A commented Mar 10, 2017

dsyme commented Mar 10, 2017

dsyme commented Mar 10, 2017 • edited Loading

KevinRansom commented Mar 10, 2017 via email

KevinRansom Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricosada commented Mar 10, 2017

KevinRansom commented Mar 10, 2017

dsyme commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricosada Mar 10, 2017 •

edited

Loading

dsyme commented Mar 10, 2017 •

edited

Loading

KevinRansom commented Mar 10, 2017 •

edited

Loading

dsyme commented Mar 10, 2017 •

edited

Loading

dsyme commented Mar 10, 2017 •

edited

Loading

enricosada commented Mar 10, 2017 •

edited

Loading

enricosada commented Mar 10, 2017 •

edited

Loading

dsyme commented Mar 10, 2017 •

edited

Loading

KevinRansom Mar 10, 2017 •

edited

Loading

dsyme commented Mar 10, 2017 •

edited

Loading