dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.92k stars 785 forks source link

Add FSharp.Build support for reference assemblies #13223

Open vzarytovskii opened 2 years ago

vzarytovskii commented 2 years ago

Add or modify FSharp.Build tasks that will determine if projects need to be re-built or not depending on if the reference assembly output has changed.

See Roslyn's implementation of this: dotnet/roslyn@315c2e1/src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs

kerams commented 2 years ago

Any chance of this making it into .NET 7?

vzarytovskii commented 2 years ago

I believe it's already there, probably has issues though, since we didn't do a comprehensive testing.

NinoFloris commented 2 years ago

Seems like RC2 will contain the enabling bits https://github.com/dotnet/sdk/pull/26306

vzarytovskii commented 2 years ago

Seems like RC2 will contain the enabling bits dotnet/sdk#26306

This is SDK support, FSharp.Build support went in earlier: https://github.com/dotnet/fsharp/pull/13567 https://github.com/dotnet/fsharp/pull/13263

NinoFloris commented 2 years ago

Right, I was looking for the sdk support because that's what I've been waiting for. @kerams and I are probably commenting on the wrong issue ;)

charlesroddie commented 2 years ago

In VS 17.3.2 I find that reference assemblies seem to work between F# projects, but not F# -> C#. Is it that the feature isn't in released VS yet or is this a bug that I should post about separately?

marcin-krystianc commented 2 years ago

In VS 17.3.2 I find that reference assemblies seem to work between F# projects, but not F# -> C#. Is it that the feature isn't in released VS yet or is this a bug that I should post about separately?

There was a bug that got fixed via https://github.com/dotnet/fsharp/pull/13567, I'm not sure in which version of VS it will be included though.

safesparrow commented 1 year ago

Is this now complete?

vzarytovskii commented 1 year ago

Is this now complete?

Not fully, I believe we don't make decisions to rebuild based on MVID yet.

safesparrow commented 1 year ago

Hmm, I'm confused. This already works in multi-project scenarios, as in compilation is avoided when non-public parts of a dependency change. Which must imply that the timestamps of the reference assembly files used in dependant projects do not get updated when non-public changes happen in dependencies.

I can also see that while obj\Debug\net7.0\refint\{name}.dll changes every time, obj\Debug\net7.0\ref\{name}.dll does not, and subsequent projects use the latter as a reference.

So to me it seems like nothing is missing.

The Roslyn implementation you linked to decides whether to copy a file (it only copies it if the MVID changed). This part already works AFAICS.

vzarytovskii commented 1 year ago

Hmm, I'm confused. This already works in multi-project scenarios, as in compilation is avoided when non-public parts of a dependency change. Which must imply that the timestamps of the reference assembly files used in dependant projects do not get updated when non-public changes happen in dependencies.

I can also see that while obj\Debug\net7.0\refint\{name}.dll changes every time, obj\Debug\net7.0\ref\{name}.dll does not, and subsequent projects use the latter as a reference.

So to me it seems like nothing is missing.

The Roslyn implementation you linked to decides whether to copy a file (it only copies it if the MVID changed). This part already works AFAICS.

They're not copied, but still emitted, we can be avoiding emitting those alltogether.

I.e. We can be passing existing MVID when generating reassemblies and if matches before ilxgen, we can just not do anything.

safesparrow commented 1 year ago

They're not copied, but still emitted, we can be avoiding emitting those alltogether. I.e. We can be passing existing MVID when generating reassemblies and if matches before ilxgen, we can just not do anything.

Ok, that seems like an additional feature. It's not implemented in C#, is it, given that both C# and F# use the refint intermediate directory for always emitting a ref assembly?

The ref assembly is then conditionally copied over to ref directory if it's changed - for C# it's done in the code you linked.

vzarytovskii commented 1 year ago

They're not copied, but still emitted, we can be avoiding emitting those alltogether. I.e. We can be passing existing MVID when generating reassemblies and if matches before ilxgen, we can just not do anything.

Ok, that seems like an additional feature. It's not implemented in C#, is it, given that both C# and F# use the refint intermediate directory for always emitting a ref assembly?

The ref assembly is then conditionally copied over to ref directory if it's changed - for C# it's done in the code you linked.

Yeah, I wanted it to be an additional feature - to not even emit it if we don't have to.

T-Gro commented 1 year ago

Yeah, I wanted it to be an additional feature - to not even emit it if we don't have to.

Good news for us, there is a dedicated 'copy to final folder' step, which is clever and checks the mvid's of assemblies before/after.

  1. msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets at main · dotnet/msbuild · GitHub It is in the Common.*.targets, therefore (I hope!) this applies to all .net languages.
  2. This Task is written in Roslyn repo roslyn/src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs at main · dotnet/roslyn · GitHub and works by opening the target folder (/bin), finding the "old" .dll, and reading it's mvid from the metadata section. Doing the same for the new one, and copying only if their MVID's are different => good.

    1. The MVID value itself is not passed using msbuild, i.e. the only info passed is the target folder location, and then the possibility to load it from a file is there.

    2. If we in F# do nothing, this will work for us as well. Ref assembly will be produced, but not copied over to /bin and therefore timestamp will not get updated.

    3. If we wanted to even stop producing the intermmediate reference assembly, we would need to do the same:

    4. Load the existing one by using msbuild properties about the target folder

    5. Load it's mvid by something similar to the roslyn/src/Compilers/Core/MSBuildTask/MvidReader.cs at main · dotnet/roslyn · GitHub ( => more code in compiler)

    6. Make sure we would not be breaking anything if not producing the expected intermmediate ref assembly (this might be the hardest part)