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.93k stars 788 forks source link

ReferencePathWithRefAssemblies missing for CoreCompile during DesignTime builds in Ionide.ProjInfo #14250

Open TheAngryByrd opened 2 years ago

TheAngryByrd commented 2 years ago

👋 Hey there!

I've been running down the issue https://github.com/ionide/proj-info/issues/171 with regard to getting Ionide.ProjInfo working on dotnet 7. After doing some updates, I was able to get it to load projects but when trying to typecheck files, Fsharp.Compiler.Service would give the error:

Assembly: The system type 'System.Array'  was required but no referenced system DLL

Since System.Array is in System.Runtime.dll I started looking at the difference in dlls that ProjInfo finds With a working version it finds in in the References C:\\Program Files\\dotnet\\packs\\Microsoft.NETCore.App.Ref\\6.0.9\\ref\\net6.0\\System.Runtime.dll With a nonworking version, it doesn't find find it in the references, and just looks it up via C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.8\Facades\System.Runtime.dll.

So after doing some binlog investigation I noticed that the FSCTask for the 7 branch was not outputting any reference files.

Working .net 6 version:

image

Not working dotnet 7 version:

image

There was a change recently https://github.com/dotnet/fsharp/pull/13567 that changed the Inputs of the CoreCompile step to use ReferencePathWithRefAssemblies instead of the previous ReferencePath. Looking for where that pops up, I found over in the Roslyn targets, a shim for this:

  <Target Name="ShimReferencePathsWhenCommonTargetsDoesNotUnderstandReferenceAssemblies"
          BeforeTargets="CoreCompile"
          Condition="'@(ReferencePathWithRefAssemblies)' == ''">
    <!-- 
      FindReferenceAssembliesForReferences target in Common targets populate this item 
      since dev15.3. The compiler targets may be used (via NuGet package) on earlier MSBuilds. 
      If the ReferencePathWithRefAssemblies item is not populated, just use ReferencePaths 
      (implementation assemblies) as they are.

      Since XAML inner build runs CoreCompile directly (instead of Compile target),
      it also doesn't invoke FindReferenceAssembliesForReferences listed in CompileDependsOn.
      In that case we also populate ReferencePathWithRefAssemblies with implementation assemblies.
    -->
    <ItemGroup>
      <ReferencePathWithRefAssemblies Include="@(ReferencePath)" />
    </ItemGroup>
  </Target>

I added this to my local Microsoft.FSharp.targets file and it seems to resolve the issue.

Screenshot of it working on dotnet 7

image

I discussed this with @vzarytovskii @baronfel on FSSF Slack. I can send a PR for this shim or if anyone knows of another way to get ReferencePathWithRefAssemblies set during DesignTime related build.

Repro steps

Provide the steps required to reproduce the problem:

  1. Step A
  2. Step B

If possible attach a zip file with the repro case. This often makes it easier for others to reproduce. The zip file should ideally represent the situation just before the call/step that is problematic.

Expected behavior

Provide a description of the expected behavior.

Actual behavior

Provide a description of the actual behaviour observed.

Known workarounds

Provide a description of any known workarounds.

Related information

Provide any related information (optional):

TheAngryByrd commented 2 years ago

Well I just ended up following a thread https://github.com/dotnet/roslyn/pull/19146#discussion_r114613742 that talks about how to make sure that ReferencePathWithRefAssemblies is not empty. Adding FindReferenceAssembliesForReferences to ProjInfo seems to work.

Given that I don't think there's actually an issue here now. Feel free to close this.

TheAngryByrd commented 2 years ago

@baronfel mentioned in the issue in ProjInfo there's probably some work around aligning with Roslyn behavior as they might not be having this issue, so I'll let y'all decide if you want to use this issue or open a new one.

vzarytovskii commented 1 year ago

@KevinRansom shall we include it to our target files, so people don't need to do workarounds?

KevinRansom commented 7 months ago

@vzarytovskii yes