search

fable-compiler / Fable

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

Consider adding `--msbuild-property <key> <value>` allowing user to pass properties to MSBuild from Fable CLI #3515

Open jwosty opened 1 year ago

jwosty commented 1 year ago

Description

I have a project which has a custom Exec target to build its Fable artifacts. This works fine for Fable 4 + .NET <= 6. However when upgrading to .NET 7 or 8, my project hangs forever after this output:

.> dotnet restore MyProject.csproj -p:FABLE_COMPILER=true -p:FABLE_COMPILER_4=true -p:FABLE_COMPILER_JAVASCRIPT=true
  Determining projects to restore...
  All projects are up-to-date for restore.

After a lot of debugging, I determined that this is because it's recursively building itself via the <Exec> task.

Repro code

Put the following in your App.fsproj:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <Compile Include="App.fs" />
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="Fable.Browser.Dom" Version="2.2.0" />
    <PackageReference Include="Fable.Core" Version="3.2.3" />
  </ItemGroup>
  <Target Name="CustomPostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="touch some-file.txt" />
  </Target>
</Project>

Then, build fable artifacts: dotnet fable .

Expected and actual results

Fable probably shouldn't attempt to run the Exec target, given the fact that this didn't always happen. Instead, you will notice that some-file.txt was created.

Alternatively, if I could set custom MSBuild properties when invoking dotnet fable, I could work around this. In my real project, I already have it set up so that it skips this target if NoFableBuild is set. Due to the way my builds are set up, I really need this Fable build target to be enabled by default.

Related information

MangelMaxime commented 1 year ago

Hello @jwosty ,

Sorry to ask but are you sure that by using .NET 6 you don't have the some-file.txt file created?

I am asking because I am able to reproduce your problem on both .NET 6 and .NET 7 with fable 4.1.4

MangelMaxime commented 1 year ago

I found a potential workaround this issue.

Fable has a --define argument allowing you to pass compiler directives to be used in F# code via #if ... #endif.

But --define instructions are also forwarded to the dotnet restore command via -p:{constant}=true.

This means you can make a clever (or hacky 😇) workaroud using MSBuild conditions.

In the example below, the <Target> node is executed only if FABLE_INVOKED_FROM_MSBUILD is not provided.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <Compile Include="App.fs" />
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="Fable.Browser.Dom" Version="2.2.0" />
    <PackageReference Include="Fable.Core" Version="3.2.3" />
  </ItemGroup>
 <PropertyGroup>
    <FABLE_INVOKED_FROM_MSBUILD Condition=" '$(FABLE_INVOKED_FROM_MSBUILD)' == '' ">false</FABLE_INVOKED_FROM_MSBUILD>
 </PropertyGroup>
  <Target Condition="'$(FABLE_INVOKED_FROM_MSBUILD)'=='false'" Name="CustomPostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="dotnet fable --define FABLE_INVOKED_FROM_MSBUILD --noCache" />
  </Target>
</Project>

Note:

This is all supposition from me and here so I can refer to it when looking into your issue

I think I found the line responsible for building the project. At least let results = analyzer.Build(env) seems like a good candidate ^^.

https://github.com/fable-compiler/Fable/blob/dd4d6c986c1083495044ed29e9f9d11abe4dfd66/src/Fable.Cli/ProjectCracker.fs#L426-L442

This is strange because the analyser is run DesignTime=true which I suppose mean don't build stuff?

I don't think we can remove the analyzer.Build stuff because it is required to get the dependencies etc.

But I think we should be able to provide additional MSBuild properties like FABLE_PROJECT_CRACKING.

Which would allow you to something like:

<Target Condition="'$(FABLE_PROJECT_CRACKING)'=='false'" Name="CustomPostBuild" AfterTargets="PostBuildEvent">
MangelMaxime commented 1 year ago

Actually, it seems like there is already a DesignTimeBuild property in MSBuild or BuildAnalyser.

Using the example below is enough for me to not be able to reproduce your problem. Can you please check ?

  <Target Condition="'$(DesignTimeBuild)' == 'false'" Name="CustomPostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="touch some-file.txt" />
  </Target>
jwosty commented 1 year ago

OK, you were right that all version of Fable 4 do this. Fable 3 plus .NET 6 is actually the combination which doesn't do this. Fable 3.7.18 with .NET 7 or 8 hangs indefinitely, which is this issue: https://github.com/fable-compiler/Fable/issues/3294

Interesting; I saw that Fable passes in FABLE_COMPILER (and some other properties) and tried to use that, but it seemed to never be defined at the build step. I assumed it's because they only get defined for dotnet restore, from the project's perspective? I'll be trying out a custom property to see if that works.

MangelMaxime commented 1 year ago

OK, you were right that all version of Fable 4 do this. Fable 3 plus .NET 6 is actually the combination which doesn't do this. Fable 3.7.18 with .NET 7 or 8 hangs indefinitely, which is this issue: https://github.com/fable-compiler/Fable/issues/3294

This is indeed more expected to me as in order to support .NET 7 which was introduce in Fable 4, the ProjectCraker has been changed completely.

MangelMaxime commented 1 year ago

Sorry for the spam,

I just saw a note on https://github.com/dotnet/project-system/blob/main/docs/design-time-builds.md

Which says:

NOTE: The DesignTimeBuild property is typically empty ('') in normal builds, so avoid comparisons to 'false'.

So you probably want to use Condition="'$(DesignTimeBuild)' != 'true'" to avoid the comparisons with false.

jwosty commented 1 year ago

Actually, it seems like there is already a DesignTimeBuild property in MSBuild or BuildAnalyser.

Using the example below is enough for me to not be able to reproduce your problem. Can you please check ?

NOTE: The DesignTimeBuild property is typically empty ('') in normal builds, so avoid comparisons to 'false'.

So you probably want to use Condition="'$(DesignTimeBuild)' != 'true'" to avoid the comparisons with false.

I can confirm this works in the real project. Thank you!

Now I'm not so sure whether or not the bug is with Fable or my project file -- the part that tripped me up the most was that the behavior changed between versions in such a way that in my scenario, it manifested in the exact same way as #3294, leading me in the wrong direction (probably chalk that up to a very unfortunate coincidence that not many people will run into). It could definitely be argued that it's really a bug in my project file, which shouldn't perform a Fable build on itself during design-time, especially since we have $(DesignTimeBuild).

If that ends up being the case, I would recommend that Fable at least print <Message> elements, to make debugging this kind of thing easier.

At the very least I can say that it's amazing to finally have any solution at all to this. I've been trying to upgrade my project since .NET 7 came out, and this has been the hurdle for a year or two at this point (there were other problems related to FAKE for a while too, but I've gotten around those eventually). Yay!

jwosty commented 1 year ago

Either way, it would also be nice to have an official bespoke --property argument, separate from --define. Not high priority, though, since there's a better workaround.

Also just want to add that I appreciate all the work y'all do on Fable on a regular basis. I really love using it in my stuff.

MangelMaxime commented 1 year ago

If that ends up being the case, I would recommend that Fable at least print elements, to make debugging this kind of thing easier.

The thing is that I don't think the bug is in Fable but either in the BuildAnalyser that we use to parse project files or in MSBuild.

I also don't think we can identify this problem on Fable side, because I suspect that what is happening is an infinite loop of Fable being spawned by BuildAnalyser in this case.

For now I think using $(DesignTimeBuild) is a good solution and in the future introducing --msbuild-property <key> <value> could be possible.

This will probably happen when switching Fable custom args parser to something like Argu. But that a whole project in it self because Fable CLI is not standard at all and I worry that we would need to introduce breaking changes in the CLI in order to standard on good basis again ...