Removing default ProduceReferenceAssembly value for F#

[marcin-krystianc]

Generation of reference assemblies for F# was disabled previously (#13085) because the F# compiler didn't support them.
Now both, the F# compiler (dotnet/fsharp#12334) and the FCS task (dotnet/fsharp#13263) support reference assemblies. Also, the bug around the copying of F# ref assemblies was fixed (dotnet/fsharp#13266) so I think it is time to enable the generation of ref assemblies for F#.

[baronfel]

We can't take this in main, because the version of F# referenced in this repo hasn't advanced to the point where that is available. I think F# is doing codeflow from the 17.3 branch into the release/6.0.4xx branch here, and that branch is good to go. Mind retargeting this PR to that branch? You can check the fsharp commit used in the eng/Versions.Details.props file in this repo.

[marcin-krystianc]

@baronfel I've rebased my branch on release/6.0.4xx but the tests still fail as if F# compiler didn't support reference assemblies. I've checked that the version from Version.details.xml contains all necessary changes so I'm not sure what is going on. I'll try to debug the problem locally.

[marcin-krystianc]

@baronfel I've confirmed that locally it works fine and the GivenThatWeWantToBuildADesktopExeWithFSharp.It_builds_a_simple_net50_app runs successfully. So it means that release/6.0.4xx branch references F# compiler that is recent enough.
I've noticed though, that the CI failures are connected to the F# compiler version shipped together with Visual studio instead of .NET SDK (see https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-sdk-refs-pull-26306-merge-3002e1399eb341d880/Microsoft.NET.Build.Tests.dll.4/2/console.3dfa9059.log?helixlogtype=result):

 CoreCompile:
        C:\Program Files\Microsoft Visual Studio\2022\Preview/Common7/IDE/CommonExtensions/Microsoft/FSharp/Tools/fscAnyCpu.exe -o:obj\Debug\net5.0\TestApp.dll
...

Does it mean that we need to wait for newer release of Visual Studio before we can merge this PR?

[baronfel]

I think so - specifically that the F# team inserts a version of their tools that is compatible with this into VS previews. @vzarytovskii do you have any insight into that schedule?

[vzarytovskii]

I think so - specifically that the F# team inserts a version of their tools that is compatible with this into VS previews. @vzarytovskii do you have any insight into that schedule?

I try competing insertions every day, i can't look into the error now, we have public holidays today and tomorrow. I will take a look at it later this week.

[vzarytovskii]

I'm not entirely sure I understand the error fully. @marcin-krystianc can you please help me here - does the test try to produce refassembly and then doesn't find it on disk? What version of sdk/vs does it use? It may not include needed FSharp.Build/compiler changes yet.

[marcin-krystianc]

It may not include needed FSharp.Build/compiler changes yet.

The test simply tries to build an F# app. After my change, the ProduceReferenceAssembly is going to be set to true by default. The build fails because the FSC task doesn't generate right parameters for the compiler so the reference assembly isn't generated but it is expected to be generated.

From my undressing from the logs, the problem is because the test pipeline uses VS version that predates the dotnet/fsharp#13263. But I don't know where to check for the version of VS being used in these tests.

[vzarytovskii]

From my undressing from the logs, the problem is because the test pipeline uses VS version that predates the dotnet/fsharp#13263. But I don't know where to check for the version of VS being used in these tests.

Yep, this is a likely cause. It was merged to the 17.3 by now, and should be in the next preview.

[nojaf]

@vzarytovskii @baronfel looking at https://docs.microsoft.com/en-us/visualstudio/releases/2022/release-notes-preview,there appears to be a preview release on June 14th.

As dotnet/fsharp#13263 was merged on June 9th, would that 17.3 preview 2 contain what we need? If so, how can we help?

[vzarytovskii]

@vzarytovskii @baronfel looking at https://docs.microsoft.com/en-us/visualstudio/releases/2022/release-notes-preview,there appears to be a preview release on June 14th.

As dotnet/fsharp#13263 was merged on June 9th, would that 17.3 preview 2 contain what we need? If so, how can we help?

It may not have it, since the "freeze" is usually a bit earlier than the preview releases.

[nojaf]

Hey @marcin-krystianc, could you retarget this PR to the 7.0.1xx-preview7 branch.
Given that we found out about dotnet/fsharp#13567 today, I believe we missed our window for the 6.0.400 release.

[marcin-krystianc]

Hey @marcin-krystianc, could you retarget this PR to the 7.0.1xx-preview7 branch.

Done.

[marcin-krystianc]

From the logs, I can conclude that the failing tests run on Windows.Amd64.VS2022.Pre.Open. From https://helix.dot.net/#Test-Internal-Windows I can see that it contains VS 17.3.0-pre.2 (released June 14, 2022).
I tried to install it locally to see whether the Microsoft.FSharp.Targets contains changes we need for this PR, but I don't know how to install old preview versions. I could only install preview 5, for which I verified that it contains necessary Microsoft.FSharp.Targets changes.

Does anyone knows whether it is possible to install the "preview 2" version to check the Microsoft.FSharp.Targets that is shipped with it? Or when we can expect the latest VS preview (at least 5) to be used for testing?

[dsplaisted]

From the logs, I can conclude that the failing tests run on Windows.Amd64.VS2022.Pre.Open. From https://helix.dot.net/#Test-Internal-Windows I can see that it contains VS 17.3.0-pre.2 (released June 14, 2022). I tried to install it locally to see whether the Microsoft.FSharp.Targets contains changes we need for this PR, but I don't know how to install old preview versions. I could only install preview 5, for which I verified that it contains necessary Microsoft.FSharp.Targets changes.

Does anyone knows whether it is possible to install the "preview 2" version to check the Microsoft.FSharp.Targets that is shipped with it? Or when we can expect the latest VS preview (at least 5) to be used for testing?

@MattGal @marcpopMSFT for updating the VS preview used for helix images.

[MattGal]

@MattGal @marcpopMSFT for updating the VS preview used for helix images.

Just for clarification, yesterday this queue was taken from 17.3.0-preview 1.1 to preview 2. After approximately two weeks this will be bumped to the version currently in "Scouting", preview 4. I will send you offline a repo to check commit history for where to get the standalone installers.

[MattGal]

Discussed with @dsplaisted , we have a DevTest Lab setup for repro VMs if you want to investigate, no older installers required. Ping me on Teams if you need this.

[nojaf]

Hello @dsplaisted and @MattGal, is there any news of the update of the Visual Studio Preview on the Helix image?

[nojaf]

Hi @MattGal, yes I think we are there.
Everything is green now, can we still target dotnet:release/7.0.1xx-rc1?

[MattGal]

Hi @MattGal, yes I think we are there. Everything is green now, can we still target dotnet:release/7.0.1xx-rc1?

Probably? I'm only here for build and test infrastructure concerns, I'd defer to @marcpopMSFT for questions about what commits can go into SDK release branches.

[baronfel]

@nojaf this is too late for rc1, we're very far past code freeze at this point. This should be retargeted to release/7.0.1xx for inclusion in RC2.

[marcin-krystianc]

@nojaf this is too late for rc1, we're very far past code freeze at this point. This should be retargeted to release/7.0.1xx for inclusion in RC2.

Done

[dsplaisted]

Is there any concern that this could break projects (most likely with custom MSBuild logic) that weren't expecting reference assemblies to be enabled? For C#, we only enabled reference assemblies for .NET 5 and up, so that it would only affect new or retargeted projects.

[baronfel]

@marcin-krystianc I chatted with @vzarytovskii about @dsplaisted's concern and we agree that this should be gated by a TFM upgrade on the user's part. You should be able to follow the pattern done for this property previously, checking for 7.0 instead of 5.0.

[marcin-krystianc]

@marcin-krystianc I chatted with @vzarytovskii about @dsplaisted's concern and we agree that this should be gated by a TFM upgrade on the user's part. You should be able to follow the pattern done for this property previously, checking for 7.0 instead of 5.0.

Sure, I will do that. Doesn't it mean though, that for netstandard targets the ProduceReferenceAssembly is never going to be set to true? It seems that it is suboptimal as the main advantage of reference is to be used with library projects which are likely to use netstandard targets.

[baronfel]

I think that's correct, based on my reading and based on a sample binlog I just made. I think library authors that want a ref assembly are just used to turning it on explicitly via property. We should align with that precedent.

[dsplaisted]

Library authors shouldn't necessarily be targeting .NET Standard, especially as time goes on: https://devblogs.microsoft.com/dotnet/the-future-of-net-standard/#what-you-should-target

[marcin-krystianc]

@marcin-krystianc I chatted with @vzarytovskii about @dsplaisted's concern and we agree that this should be gated by a TFM upgrade on the user's part. You should be able to follow the pattern done for this property previously, checking for 7.0 instead of 5.0.

Hmm, actually it is quite complicated. I cannot follow the same pattern in the Microsoft.NET.Sdk.FSharp.props file as it is being included before the TargetFrameworkIdentifier is even determined ( via Microsoft.NET.TargetFrameworkInference.targets). So the condition for ProduceReferenceAssembly would never be evaluated to true and the definition from Microsoft.NET.TargetFrameworkInference.targets would be still used. Therefore the ProduceReferenceAssembly would be true for net5.0 and later.

Maybe we need to update Microsoft.NET.TargetFrameworkInference.targets file so it checks whether the project is F# project and use 7.0 version in the condition for F# projects, but keep using 5.0 for other project types as it does currently?

[baronfel]

That seems reasonable to me, thanks for the explanation.

[marcin-krystianc]

That seems reasonable to me, thanks for the explanation.

Ok, I've implemented it following my previous description.

[baronfel]

I wish we didn't have to use .fsproj as the comparison here. I was going to suggest using $(Language) instead, but that's not set until Microsoft.FSharp.targets, well after this file is imported.

[baronfel]

@marcin-krystianc when you get a moment, adding a test around this would be really nice - we have several that do evaluations around these sorts of props, and it would be a nice backstop.

[marcin-krystianc]

@marcin-krystianc when you get a moment, adding a test around this would be really nice - we have several that do evaluations around these sorts of props, and it would be a nice backstop.

Sure, will give it a try.