Run design time builds out-of-process #69616
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
from
69e1b40
to
151fedf
Compare
sharwell
reviewed
Aug 22, 2023
...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BuildHostProcessManager.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
4 times, most recently
from
69a69e9
to
8f6d151
Compare
jasonmalinowski
commented
Aug 25, 2023
| @@ -157,7 +156,7 @@ private async Task<bool> TryEnsureMSBuildLoadedAsync(string workingDirectory) | |||
| if (msbuildInstance != null) | |||
| { | |||
| MSBuildLocator.RegisterInstance(msbuildInstance); | |||
| _logger.LogInformation($"Loaded MSBuild at {msbuildInstance.MSBuildPath}"); | |||
| _logger.LogInformation($"Loaded MSBuild in-process from {msbuildInstance.MSBuildPath}"); | |||
There was a problem hiding this comment.
We are still loading an MSBuild version in-process for reading solution files. A bit silly at this point, but I clarified the text to make it less confusing.
jasonmalinowski
commented
Aug 25, 2023
...anguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BuildHostProcessManager.cs
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
from
d35d600
to
9208679
Compare
This moves nearly all of the code that uses MSBuild to a separate project, Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost. This new project is an executable project, and will eventually be the separate process that is invoking MSBuild. For now, it's still consumed as a regular project-to-project reference to keep everything else working. There are a few things that are still using MSBuild in the original library, namely that we have some methods that take an ILogger and the OpenSolutionAsync method calls into the solution file parsing APIs. The method that takes an ILogger we will not be able to support in an out-of-proc mode (although we can still maintain an in-proc compat mode); the OpenSolutionAsync we will probably be able to address by having the solution file parsing happening in-process since that doesn't depend on any project file details.
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
from
9208679
to
e9fea77
Compare
The shape of this type was a bit deceiving: a ProjectFile has a Log property, and this also having a Log property makes it appear like there's one log for the basic loading of the project, and then a second log for the actual loading for a specific TFM. This is however not the case: ProjectFileInfo.Log would always be the same instance as the ProjectFile it came from. Removing this is easy, as the only place it was ever used was where we already had the ProjectFile in hand, and it makes ProjectFileInfo a simple data type.
This implements the basic launching of the build host process, setting up the RPC channel, and running the design-time builds in that process. Right now this only works for .NET Core projects.
Our rule for project files is two space indent, this was using four.
This is a hack that's inspired from some of the similar things we do in our interactive window tests where we want to deploy multiple versions there too.
This implements the selection algorithm, and will launch the .NET Framework build host if needed.
Besides passing through the option (which we pass as a switch to the build host process), this also fixes up dispose handling so we properly shut down the build host. Otherwise we might not have fully flushed the binary logs in the first place.
This environment variable is set by MSBuildLocator to the version of MSBuild that is located and registered in a process. We don't want this to ever be inherited to a build host, since the whole point is to have a different MSBuild version there.
In the .NET Core case, we still want to consider the project path to ensure we have the right global.json. Once we can do this detection in the main process then we can remove this and simplify it again.
This was referenced Aug 30, 2023
Open
Open
Open
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
from
f60c2f6
to
4a152e8
Compare
Right now our LanguageServer NuGet package is too big to upload, so this reduces the total size of the package by around 40 MB, but also simplifies some things and speeds it up too. Previously we were using a ProjectFileLoaderRegistry to discover the IProjectFileLoader for a given project; this does a bunch of MEF stuff to find the loader, allow for custom associations to file types, and also ensures we have the other workspace binaries for that language. But IProjectFileLoader isn't public, so at most there could ever be just two: C# and VB. But we were still MEF composing all of that to simply discover those two types and also ensure we had the right workspace binaries deployed to parse the resulting compiler command line -- even though it's not used in the BuildHost process. But if we simply skip all of that and just replace it with a switch statement and a check in-process that we have a ICommandLineParserService, we can remove the need for MEF entirely, and also stop deploying the C# binaries altogether, greatly simplifying the BuildHost code. Although it might not really be necessary long term, I'm moving the ProjectFileLoaderRegistry and DiagnosticReporter back to the main MSBuildWorkspace assembly since we don't need it in the BuildHost either anymore.
This will allow us to also create BuildHost objects in-proc as a fallback scenario.
dibarbet
reviewed
Aug 30, 2023
| private void Process_ErrorDataReceived(object sender, DataReceivedEventArgs e) | ||
| { | ||
| if (e.Data is not null) | ||
| LoggerForProcessMessages?.LogTrace($"Message from Process: {e.Data}"); |
There was a problem hiding this comment.
Something to consider for later - similar to LSP trace logging, sometimes the json itself is useful to log at the most verbose levels. In case there are weird serialization issues.
dibarbet
approved these changes
Aug 30, 2023
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Show resolved
Hide resolved
This is only needed as a compatibility switch if we have a regression with the new support.
jasonmalinowski
force-pushed
the
out-of-proc-design-time-builds
branch
from
c4a411d
to
b2cc3b8
Compare
Trying to ngen this produces some errors because we're not shipping the dependencies that are used for the console-application portion of it (like parsing command line switches and locating an MSBuild in that process). But we don't have any need for that, so we can just disable ngen to silence errors.
ghost
added this to the 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements out-of-proc design time builds for the LSP language server (i.e. C# extension), which should bring:
This turned out to be relatively straightforward to do: our MSBuildWorkspace code (for the most part) can be split into two pieces:
By a bit of luck, when I implemented the LSP server project system I was only calling into the first bits as a way to get command line arguments and document lists, and from there passed that to other systems to create the workspace. And it turned out that the data structures returned from the first part (namely ProjectFileInfo) was almost serializable -- just removing one field and everything could be sent over the wire via StreamJsonRpc. Therefore, this PR splits the stuff that talks to MSBuild (but not the workspace) into a separate executable project named Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost, and makes that bit of code callable via a StreamJsonRpc link over stdout. Then, the language server reimplements the logic VS for Windows uses to pick which project system to use, and launches the right process (.NET Core vs. .NET Framework) and then calls over via StreamJsonRpc.
For the language server, this has some extra impacts on packaging: we now pack the net472 version of the build host in a subdirectory of the main layout so that way we can launch that process. For the .NET Core build host, we just package the additional DLL and run that with dotnet Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.dll.
Right now this is not adding support for launching processes into any user of MSBuildWorkspace, at least not yet. To keep MSBuildWorkspace working, I'm relying on the (evilish) fact that an .exe (or .dll in the .NET Core case) can still be referenced as 'regular' class library for purposes of loading types from it. So anybody using MSBuildWorkspace via NuGet will now have the BuildHost deployed, but it'll just get loaded as another binary for purposes of in-proc builds. Fixing that means ensuring that the MSBuildWorkspace NuGet package would package both flavors of the build host (regardless of the runtime being consumed) and then from there launch the right one. That's absolutely work I want to do, but I want to land this safely before we start tinkering there since right now the process launching code is making specific assumptions about the layout in the LanguageServer artifacts. There is also some extra layering cleanup that needs to be done: some of the stuff that creates the projects in the workspace itself is a bit more tied to the MSBuild types and that needs to be separated a bit more.
There's also some improvements to be made in the BuildHost process in terms of reducing its disk layout size: right now for the net472 version we'll deploy the C# compiler and workspace binaries...even though we're not really using them. We are also creating a MEF container to check for the existence of some language services but I don't believe they're actually in use -- we could drop MEF entirely and refactor a bit and move some of the "is this language supported?" to the workspace (in-proc) side. I'm deferring those refactorings for now.
Things to Do
Things to Test
Things to Do Later™
This is out of scope for this PR but the intent is still to follow up and do it soon. Since there's enough users who are getting benefit from what we have here, it doesn't necessarily make sense to hold up this PR on this work.
Fixes dotnet/vscode-csharp#6126
Fixes dotnet/vscode-csharp#6250
Fixes dotnet/vscode-csharp#6216
Closes dotnet/vscode-csharp#5721