fable-compiler / Fable

F# to JavaScript, TypeScript, Python, Rust and Dart Compiler
http://fable.io/
MIT License
2.86k stars 296 forks source link

A word about ProjectCracker #3613

Open nojaf opened 7 months ago

nojaf commented 7 months ago

Hello, as I'm exploring some options to achieve https://github.com/fable-compiler/Fable/issues/3552, I'm also learning about the internals of ProjectCracker.

I knew Fable needed the F# source files in certain scenarios to compile NuGet dependencies. The documentation mentions putting all source files in the fable PackagePath together with a fsproj.

getFullProjectOpts appears to be the entry point of doing the actual cracking. It will crack the main fsproj and detect fable libraries among the references in tryGetFablePackage. This code really goes and scans the extracted NuGet package in search of a fable folder. Find the fsproj and collect the source files.

Let's imagine we are using Thoth.Json in our project.

<PackageReference Include="Thoth.Json" Version="10.2.0" />

To construct the FSharpProjectOptions, Fable will inject the source files from Thoth.Json

  <ItemGroup>
    <Compile Include="Types.fs" />
    <Compile Include="Decode.fs" />
    <Compile Include="Encode.fs" />
    <Compile Include="Extra.fs" />
  </ItemGroup>

There is a lot of custom logic involved to pull this off, and I'm wondering if we shouldn't just embrace MSBuild a little more in this case.

Locally I created Thoth.Json v13 with the following change:

If the NuGet package were to add a build/<package-id>.props file, it would be very convenient to include the source files to the actual fsproj:

image

You really only want to include the source files when Fable is evaluating the project. Inside the IDE you still want to rely on the binary to get IntelliSense.

Inside my fsproj I would add Thoth.Json like this:

  <PackageReference Include="Thoth.Json" Version="13.0.0">
    <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
  </PackageReference>

Alright, to see the results of this in action:

Running dotnet build -bl /p:DefineConstants=FABLE_COMPILER --no-incremental -v:n shows us the following CoreCompile output:

CoreCompile:
         C:\Program Files\dotnet\dotnet.exe "C:\Program Files\dotnet\sdk\8.0.100\FSharp\fsc.dll" -o:obj\Debug\net6.0\App.dll
         -g
         --debug:portable
         --noframework
         --define:FABLE_COMPILER
         --define:NET5_0_OR_GREATER
         --define:NET6_0_OR_GREATER
         --define:NETCOREAPP1_0_OR_GREATER
         --define:NETCOREAPP1_1_OR_GREATER
         --define:NETCOREAPP2_0_OR_GREATER
         --define:NETCOREAPP2_1_OR_GREATER
         --define:NETCOREAPP2_2_OR_GREATER
         --define:NETCOREAPP3_0_OR_GREATER
         --define:NETCOREAPP3_1_OR_GREATER
         --optimize-
         --tailcalls-
         -r:C:\Users\nojaf\.nuget\packages\fable.core\4.2.0\lib\netstandard2.0\Fable.Core.dll
         -r:C:\Users\nojaf\.nuget\packages\fsharp.core\7.0.400\lib\netstandard2.1\FSharp.Core.dll
         -r:C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.25\ref\net6.0\Microsoft.CSharp.dll
         -r:C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.25\ref\net6.0\Microsoft.VisualBasic.Core.dll
         -r:C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.25\ref\net6.0\Microsoft.VisualBasic.dll
         ... (more -r: SDK references) ...
         --target:library
         --warn:3
         --warnaserror:3239
         --fullpaths
         --flaterrors
         --highentropyva+
         --targetprofile:netcore
         --nocopyfsharpcore
         --deterministic+
         --simpleresolution
         obj\Debug\net6.0\.NETCoreApp,Version=v6.0.AssemblyAttributes.fs
         obj\Debug\net6.0\App.AssemblyInfo.fs
         C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\build\../fable/Types.fs
         C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\build\../fable/Decode.fs
         C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\build\../fable/Encode.fs
         C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\build\../fable/Extra.fs
         App.fs

I'm including the source files for Thoth.Json before my own code. And I don't have the -r:Thoth.Json.dll.

When running without /p:DefineConstants=FABLE_COMPILER I get:

       CoreCompile:
         C:\Program Files\dotnet\dotnet.exe "C:\Program Files\dotnet\sdk\8.0.100\FSharp\fsc.dll" -o:obj\Debug\net6.0\App.dll
         -g
         --debug:portable
         --noframework
         --define:TRACE
         --define:DEBUG
         --define:NET
         --define:NET6_0
         --define:NETCOREAPP
         --define:NET5_0_OR_GREATER
         --define:NET6_0_OR_GREATER
         --define:NETCOREAPP1_0_OR_GREATER
         --define:NETCOREAPP1_1_OR_GREATER
         --define:NETCOREAPP2_0_OR_GREATER
         --define:NETCOREAPP2_1_OR_GREATER
         --define:NETCOREAPP2_2_OR_GREATER
         --define:NETCOREAPP3_0_OR_GREATER
         --define:NETCOREAPP3_1_OR_GREATER
         --optimize-
         --tailcalls-
         -r:C:\Users\nojaf\.nuget\packages\fable.core\4.2.0\lib\netstandard2.0\Fable.Core.dll
         -r:C:\Users\nojaf\.nuget\packages\fsharp.core\7.0.400\lib\netstandard2.1\FSharp.Core.dll
         ... (more -r: SDK references) ...
         -r:C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.25\ref\net6.0\System.Xml.XPath.XDocument.dll
         -r:C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\lib\netstandard2.0\Thoth.Json.dll
         -r:C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.25\ref\net6.0\WindowsBase.dll
         --target:library
         --warn:3
         --warnaserror:3239
         --fullpaths
         --flaterrors
         --highentropyva+
         --targetprofile:netcore
         --nocopyfsharpcore
         --deterministic+
         --simpleresolution
         obj\Debug\net6.0\.NETCoreApp,Version=v6.0.AssemblyAttributes.fs
         obj\Debug\net6.0\App.AssemblyInfo.fs
         App.fs

Notice I don't have the Thoth.Json source files but I do have the -r:C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\lib\netstandard2.0\Thoth.Json.dll.

This moves a lot of logic from ProjectCracker to MSBuild which is good at these sorts of project system evaluations.

I'm quite sure this would be beneficial for @jwosty in https://github.com/fable-compiler/Fable/issues/3549. As of right now, this is a limitation to piggyback on MSBuild.

Going in this direction would of course be a breaking change to the Fable ecosystem. However, it opens the door to having a better grasp on project options extraction. This would allow us to troubleshoot things easily from binary logs.

The breaking changes on the author side would be to include a props file and on the consumer side to ExcludeAssets compile for Fable packages.

I'm assuming someone will make the argument that fable-standalone probably can't rely on this. That may be a fair critique, however, I don't think it stands. The gain of using MSBuild is significant for what I expect to be 95% of Fable's current use case, so I think it makes sense that fable-standalone figures things out in another way.

@baronfel am I making sense here MSBuild-wise?

baronfel commented 7 months ago

Yep, this makes sense to me. A couple notes:

nojaf commented 7 months ago

how does this automatic detection work with package ordering?

Did a quick local test:

When adding:

  <ItemGroup>
    <PackageReference Include="Fable.Core" Version="4.2.0" />
    <PackageReference Include="Thoth.Json" Version="13.0.0">
      <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
    </PackageReference>
    <PackageReference Include="FableLibB" Version="1.0.0">
      <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
    </PackageReference>
    <PackageReference Include="FableLibC" Version="1.0.0">
      <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
    </PackageReference>
  </ItemGroup>

The files of A are included, so it does not need a direct reference to A. Assume package A used the buildTransitive folder to pack the props file!

The order I added A in does not matter, it will only include the files of A once before the files of B and C.

nojaf commented 7 months ago

@baronfel turns out you can put the props file in the buildTransitive and that way if will be evaluated for A as well. Works like a charm 🪄

MangelMaxime commented 7 months ago

If I understand correctly, it means that the user needs to write:

  <PackageReference Include="Thoth.Json" Version="13.0.0">
    <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
  </PackageReference>

in their WebApp.fsproj correct?

How does this works with paket?

We also need to check that this works with MSBuild central package management.

nojaf commented 7 months ago

If I understand correctly, it means that the user needs to write

Yes, although we can drop the ExcludeAssets and filter that out programmatically in ProjectCracker. That wouldn't be the end of the world and is still fairly trivial. We get the project options of the main project, all files will still be in place and we just filter the -r: of Fable packages.

How does this works with paket?

It works with Paket, except that you cannot do the <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets> trick as you can with NuGet. It worked fine on my machine. Again, not being able to do this can be resolved by filtering out the references.

We also need to check that this works with MSBuild central package management.

I don't see any problem there as that merely controls the version of the PackageReference.

Overall, I'm still optimistic about this idea. Not doing the ExcludeAssets bit means end-users don't have to do anything special when upgrading (besides bumping package versions).

PS: A better MSBuild way to construct the source file path: <Project> <ItemGroup Condition="$(DefineConstants.Contains('FABLE_COMPILER'))"> <CompileBefore Include="$([MSBuild]::NormalizePath($(MSBuildThisFileDirectory), '../fable/FableLibA.fs'))"/> </ItemGroup> </Project>
MangelMaxime commented 7 months ago

Overall, I'm still optimistic about this idea. Not doing the ExcludeAssets bit means end-users don't have to do anything special when upgrading (besides bumping package versions).

Personally, I like that more as all his hidden for the end user. From experience, the more streamline things are the better.

Another question I have in regard to:

<ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>

is what does the compile refers to here? I looked at the documentation of ExcludeAssets but I am not 100% sure.

I'm assuming someone will make the argument that fable-standalone probably can't rely on this. That may be a fair critique, however, I don't think it stands. The gain of using MSBuild is significant for what I expect to be 95% of Fable's current use case, so I think it makes sense that fable-standalone figures things out in another way.

Having fable-standalone figures things out in another way is ok to me, however we do need to have a solution for fable-standalone because this is used by Fable REPL and this is a really useful tool for prototype and most importantly bug reports too.

If we are able to find the package folder, I suppose fable-standalone could read the build.props and make guesses based on that file content. Making it understand $(MSBuildThisFileDirectory) should not be too difficult I suppose.

MangelMaxime commented 7 months ago

If we are going down this road, we will also need to think how we want the transition period to be done.

  1. No transition period and directly go with Fable 5. We should also add some code in Fable 4, to detect the a library is using the new project system and inform the user that he needs to use Fable 5. This is similar to the error I generated in Fable 3 for informing the user that .NET 7 support is only in Fable 5.
  2. Have Fable 4 supports both ways of doing it and have it generates a warning that some dependencies are using the old project structure that will be dropped in Fable 5. This would allows people to create issues on the different project so the maintainers can upgrade it.

I also believe we should consider retrieving the Fable packages from NuGet kind of like https://fable.io/packages does, and test for libraries that includes fable folder, so we can create issues on the different project to upgrade them

nojaf commented 7 months ago

is what does the compile refers to here?

See https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets,

"Contents of the lib folder and controls whether your project can compile against the assemblies within the folder"

Having fable-standalone figures things out in another way is ok to me,

Yeah, I agree it should definitely still work. We should find a good way to extract where standalone will do its own resolution.

If we are going down this road, we will also need to think how we want the transition period to be done.

Yup, I would also assume having a Fable 5 would allow us to address some opportunities that benefit from making a breaking change.

jwosty commented 7 months ago

As already mentioned, I think #3549 has some overlapping goals. I was already mulling over potential breaking changes in package structure that would help it - we could take this as an opportunity to rethink packages a little bit. A custom TargetFramework would get rid of that conditional ugliness that @nojaf is having to use right now. It would be completely transparent to the Fable user.

I like the idea of doing this for Fable 5, and adding some forward compatibility to Fable 4 depending on how this shapes up