Switch MSBuildWorkspace to running builds out of proc #70469

jasonmalinowski · 2023-10-19T23:39:04Z

This switches MSBuildWorkspace over to using out-of-proc builds using the same mechanism we use for our LSP process. This brings a few advantages:

Applications on .NET Framework can now analyze .NET Core projects and vice versa without special work on the application author's side. Today your best workaround is to do something like multi-target your app for both platforms and hope the user knows which one to pick.
It ensures we're better isolated from the application's deployed binaries. For example, applications could now deploy MSBuild as a regular library without breaking MSBuildLocator (since that's not needed in proc), which simplifies things like our LSIF generator. This also fixes Workspace failed with could not load "NuGet.Frameworks" error #61454 (and similar bugs) where an extra binary being deployed can potentially break MSBuild. There's further isolation work to still do here, to ensure we're more isolated even against what the BuildHost ships, but since it means the environment for running stuff is far more controlled, we'll have more confidence of what we test in our own tests will represent the behavior in the wild.
It ensures that the user doesn't have to worry about things like MSBuildLocator or binding redirects, etc. For example, we always have a non-stop stream of questions on StackOverflow of people having issues this way and those are fixed for good.

The one major caveat here is we did have some entrypoints for MSBuildWorkspace that accept an ILogger; currently these will be broken. I'm still reaching out to folks to decide what to do here. We can simply obsolete them, or try to get them to work in a few scenarios. (For example, I could detect if the logger you passed is a well known built in logger we can just recreate that on the OOP side). This will also break any apps that were really expecting the build to be in-proc (like by tinkering with MSBuild properties in advance) but such a behavior wasn't really supported anyways.

Work still to do prior to merge:

Add support back for TryApplyChanges; I have that commented out at the moment while I got the core stable first.
Replace StreamJsonRpc with a different RPC mechanism since that's not source build friendly.
Fix some bugs around locked/corrupted project files that the new code isn't dealing with as well.
Add support back for setting properties like debug/release configurations.
Write a bunch more tests for the StreamJsonRpc replacement.

Things that will be tracked for follow-up:

Right now we are shipping two BuildHost layouts as a part of any use, which means duplicating the BuildHost and all it's references twice. We can definitely trim down number of binaries to reduce the disk size. For example we're shipping Microsoft.CodeAnalysis.Workspaces and Microsoft.CodeAnalysis mostly for various helpers or trivial enums that we could either link or just remove.
Investigate the logic of MSBuildWorkspace's support for adding documents, which seems to be repeatedly converting between relative and absolute paths.
Unit tests for metadata reference removal, since there aren't any today.
More formally obsolete the ILogger APIs.
Switching the OOP serialization to System.Text.Json so that way we're not shipping Newtonsoft.Json. Ironically we're shipping both libraries at the moment...

jasonmalinowski · 2023-10-28T01:58:48Z

src/Tools/AnalyzerRunner/AnalyzerRunnerHelper.cs

-#if NETCOREAPP
-                // This property ensures that XAML files will be compiled in the current AppDomain
-                // rather than a separate one. Any tasks isolated in AppDomains or tasks that create
-                // AppDomains will likely not work due to https://github.com/Microsoft/MSBuildLocator/issues/16.
-                { "AlwaysCompileMarkupFilesInSeparateDomain", bool.FalseString },
-#endif


If this is still necessary, it doesn't belong in this assembly anymore since the "is this .NET Core?" test should be in a different process. (And if it's generally necessary for XAML projects, then we should have this as a central fix.)

Some of the strings were used in the build host, and some were used by the main library; this splits them in preparation for removing the project reference entirely.

jasonmalinowski · 2023-11-07T01:39:05Z

src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

-        <Output TaskParameter="TargetOutputs" ItemName="BuildOutputInPackage" />
-      </MSBuild>
-  </Target>
-  <Target Name="GetBuildHostDebugSymbols">


I can't figure out what scenario symbol packing was happening in, so just removing it.

jasonmalinowski · 2023-11-08T01:38:20Z

eng/Versions.props

-    <!--
-      The package "Microsoft.CodeAnalysis.Analyzer.Testing" brings in an earlier version of these NuGet dependencies than
-      is expected by the NET SDK used in the Workspace.MSBuild UnitTests. In order to test against the same verion of NuGet
-      as our configured SDK, we must set the version to be the same.
-     -->


NuGet dependencies don't matter anymore, so this workaround can be removed for good.

this makes me very happy :)

jasonmalinowski · 2023-11-08T01:39:04Z

src/Features/Lsif/Generator/Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.csproj

+    <PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(MicrosoftExtensionsLoggingAbstractionsVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="$(MicrosoftExtensionsLoggingConsoleVersion)" />


These were implicitly coming along with the BuildHost ProjectReference, but now it's just explicit.

jasonmalinowski · 2023-11-08T02:19:17Z

src/Tools/IdeBenchmarks/IdeBenchmarks.csproj

@@ -12,7 +12,6 @@
  <ItemGroup>
    <PackageReference Include="BenchmarkDotNet" Version="$(BenchmarkDotNetVersion)" />
    <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" Version="$(BenchmarkDotNetDiagnosticsWindowsVersion)" />
-    <PackageReference Include="Microsoft.Build.Locator" Version="$(MicrosoftBuildLocatorVersion)" />


This PR generally goes through the repo and removes uses of MSBuild Locator that was used in places that were using MSBuildWorkspace. Those just aren't necessary anymore.

src/Workspaces/Core/MSBuild.BuildHost/BuildHost.cs

This binary is implicitly a dependency of the Razor source generator, so needs to be present for any time the generator is running and is running in design time mode. That's not going to happen usually but would happen in any MSBuildWorkspace scenario.

The tests we have here were testing the underlying code by directly creating loggers, but we don't have an MSBuild in process anymore for those tests to work. Rather than having a BuildHost specific test bed we'll just change the tests to test the end-to-end.

On .NET Framework, Path APIs in very invalid paths throw exceptions. On .NET Core, they do something reasonable and then you might just FileNotFound later when the file doesn't exist. This updates the tests accordingly.

We'll still keep the .NET Core build host around, since the process isolation still ensures binaries deployed as a part of the application don't break MSBuild.

Referencing Microsoft.Extensions.Logging.Abstractions breaks source build because 6.0.2 isn't available. But 6.0.0 is so we can just use that.

If it's already crashed and exited, then there's no point in trying to shutdown communication with it -- it's already gone.

If the process is dead we shouldn't be trying to send an RPC to it.

When we dispose a process, it fires events which causes us to remove it from the map.

jasonmalinowski · 2023-11-14T03:26:10Z

I'll be making a GitHub project later this week to track the follow-up work to this PR.

MSBuildLocator is no longer needed since dotnet/roslyn#70469 and the workarounds for NuGet.Framework can be removed too.

[main] Update dependencies from dotnet/roslyn, nuget/nuget.client - Add dependency on Microsoft.Extensions.Logging 8.0.0 match the version that roslyn wants dotnet/roslyn#71916 - Revert "Add dependency on Microsoft.Extensions.Logging 8.0.0" This reverts commit f947f38. - Add temporary reference to Microsoft.Extensions.Logging 8.0.0 - Cleanup dependencies, remove MSBuildLocator MSBuildLocator is no longer needed since dotnet/roslyn#70469 and the workarounds for NuGet.Framework can be removed too.

jasonmalinowski self-assigned this Oct 19, 2023

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 19, 2023

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch 4 times, most recently from 84d5350 to 1fa1478 Compare October 26, 2023 00:22

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch 2 times, most recently from 070b828 to 52be409 Compare October 28, 2023 00:07

jasonmalinowski commented Oct 28, 2023

View reviewed changes

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch 3 times, most recently from 6d3d5c9 to 6478bf9 Compare November 1, 2023 21:56

jasonmalinowski added 2 commits November 1, 2023 15:23

Split apart WorkspaceMSBuildResources

b4cd19e

Some of the strings were used in the build host, and some were used by the main library; this splits them in preparation for removing the project reference entirely.

Remove unused InternalsVisibleTo

8afde69

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch 2 times, most recently from ea08f53 to ebd68d6 Compare November 3, 2023 00:16

build-analysis bot mentioned this pull request Nov 3, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch from ebd68d6 to e7f2950 Compare November 4, 2023 00:47

jasonmalinowski commented Nov 7, 2023

View reviewed changes

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch 2 times, most recently from 62e635c to 79bfce5 Compare November 7, 2023 03:10

jaredpar self-assigned this Nov 7, 2023

jasonmalinowski marked this pull request as ready for review November 8, 2023 01:15

jasonmalinowski requested review from a team as code owners November 8, 2023 01:15

jasonmalinowski commented Nov 8, 2023

View reviewed changes

src/Workspaces/Core/MSBuild.BuildHost/BuildHost.cs Show resolved Hide resolved

jasonmalinowski added 10 commits November 13, 2023 17:50

Fix tests that are testing invalid paths

50331ec

On .NET Framework, Path APIs in very invalid paths throw exceptions. On .NET Core, they do something reasonable and then you might just FileNotFound later when the file doesn't exist. This updates the tests accordingly.

Fix indenting of all of the TargetFrameworkVersion properties in tests

a9f50fa

Fix source build by excluding the net472 build host in that case

65694dd

We'll still keep the .NET Core build host around, since the process isolation still ensures binaries deployed as a part of the application don't break MSBuild.

Fix source build

3ad9781

Referencing Microsoft.Extensions.Logging.Abstractions breaks source build because 6.0.2 isn't available. But 6.0.0 is so we can just use that.

Add additional logging for which tests are being discovered

c723216

Don't dispose the BuildHostProcess if the process crashed

039fcb1

If it's already crashed and exited, then there's no point in trying to shutdown communication with it -- it's already gone.

Better handle unexpected shutdowns

81ca930

If the process is dead we shouldn't be trying to send an RPC to it.

Fix race where we were mutating the process list while disposing

a1f6b59

When we dispose a process, it fires events which causes us to remove it from the map.

jasonmalinowski force-pushed the move-msbuildworkspace-builds-out-of-proc branch from dd359c6 to a1f6b59 Compare November 14, 2023 01:53

jasonmalinowski merged commit 5b73439 into dotnet:main Nov 14, 2023
27 checks passed

jasonmalinowski deleted the move-msbuildworkspace-builds-out-of-proc branch November 14, 2023 03:25

ghost added this to the Next milestone Nov 14, 2023

jasonmalinowski mentioned this pull request Nov 15, 2023

Workspace failed with could not load "NuGet.Frameworks" error #61454

Closed

333fred mentioned this pull request Nov 17, 2023

[main] Update dependencies from dotnet/roslyn dotnet/sdk#36852

Merged

allisonchou mentioned this pull request Nov 17, 2023

[Automated] PRs inserted in VS build main-34317.88 #70885

Closed

RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023

jasonmalinowski mentioned this pull request Nov 29, 2023

Add tests for MSBuildWorkspace on Windows containing options with quotes in them #71021

Closed

filzrev mentioned this pull request Feb 5, 2024

MSBuildWorkspaceLoader cannot access binlog (regression in dotnet/format tests) #70674

Closed

sbomer mentioned this pull request Feb 6, 2024

--binarylog option broken dotnet/format#2094

Open

Youssef1313 mentioned this pull request Feb 21, 2024

OpenProjectAsync logger's parameter is unused (public API) #72202

Open

akoeplinger added a commit to dotnet/hotreload-utils that referenced this pull request Apr 8, 2024

Cleanup dependencies, remove MSBuildLocator

1c3d91a

MSBuildLocator is no longer needed since dotnet/roslyn#70469 and the workarounds for NuGet.Framework can be removed too.

lambdageek mentioned this pull request Apr 9, 2024

[main] Update dependencies from dotnet/roslyn, nuget/nuget.client dotnet/hotreload-utils#379

Merged

filzrev mentioned this pull request Apr 25, 2024

[Bug] .NET API bug UWP No longer supported dotnet/docfx#9494

Closed

Metalaka mentioned this pull request May 16, 2024

VS 2022 can't find .NET Framework build host after update #73854

Open

jasonmalinowski mentioned this pull request Jul 10, 2024

Dotnet build hangs while building libs.tests natively using cross built dotnet sdk dotnet/runtime#101121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch MSBuildWorkspace to running builds out of proc #70469

Switch MSBuildWorkspace to running builds out of proc #70469

jasonmalinowski commented Oct 19, 2023 •

edited

Loading

jasonmalinowski Oct 28, 2023 •

edited

Loading

jasonmalinowski Nov 7, 2023

jasonmalinowski Nov 8, 2023

dibarbet Nov 8, 2023

jasonmalinowski Nov 8, 2023

jasonmalinowski Nov 8, 2023

jasonmalinowski commented Nov 14, 2023

Switch MSBuildWorkspace to running builds out of proc #70469

Switch MSBuildWorkspace to running builds out of proc #70469

Conversation

jasonmalinowski commented Oct 19, 2023 • edited Loading

jasonmalinowski Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski Nov 7, 2023

Choose a reason for hiding this comment

jasonmalinowski Nov 8, 2023

Choose a reason for hiding this comment

dibarbet Nov 8, 2023

Choose a reason for hiding this comment

jasonmalinowski Nov 8, 2023

Choose a reason for hiding this comment

jasonmalinowski Nov 8, 2023

Choose a reason for hiding this comment

jasonmalinowski commented Nov 14, 2023

jasonmalinowski commented Oct 19, 2023 •

edited

Loading

jasonmalinowski Oct 28, 2023 •

edited

Loading