Load solutions out-of-proc #70240
Merged
Load solutions out-of-proc #70240
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
We had an option as a fallback in case of the out-of-proc build had bugs, but we haven't had any reports of bugs, so just removing it.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
jasonmalinowski
force-pushed
the
parse-solution-files-oop
branch
from
ebfbec7
to
c4b29ad
Compare
jasonmalinowski
commented
Oct 4, 2023
| public Task<bool> IsProjectFileSupportedAsync(string projectFilePath, CancellationToken cancellationToken) | ||
| public Task<bool> HasUsableMSBuildAsync(string projectOrSolutionFilePath, CancellationToken cancellationToken) | ||
| { | ||
| return Task.FromResult(TryEnsureMSBuildLoaded(projectOrSolutionFilePath)); |
There was a problem hiding this comment.
It's still funky that HasUsableMSBuildAsync() implicitly triggers an actual load, but the hope is this goes away once we can reliably do all discovery in-proc.
dibarbet
reviewed
Oct 4, 2023
| var buildHost = await buildHostProcessManager.GetBuildHostAsync(BuildHostProcessManager.BuildHostProcessKind.NetCore, CancellationToken.None); | ||
|
|
||
| // If we don't have a .NET Core SDK on this machine at all, try .NET Framework | ||
| if (!await buildHost.HasUsableMSBuildAsync(solutionFilePath, CancellationToken.None)) |
There was a problem hiding this comment.
should this logic move into GetBuildHostAsync, where the reverse fallback already lives? Would be nice if all the fallback logic was in one place
There was a problem hiding this comment.
Going to leave it since this will get refactored further in a week or two.
dibarbet
approved these changes
Oct 4, 2023
This brings a few benefits: 1. It's possible that the user might have an 8.0 SDK but only a 7.0 runtime installed; in that case, our language server will run with the 7.0 runtime, but will fail to find an MSBuild since the only one on the machine might be the 8.0 version which is not runnable. This ensures we can still function in that scenario. We'll also fall back to using a .NET Framework MSBuild if somehow that's all you have. 2. When we loaded an MSBuild, that sets environment variables, which might not be the same variables set if we're registering a different version in our child processes. But the environment variables might get inherited, which could result in surprises. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1886993
jasonmalinowski
force-pushed
the
parse-solution-files-oop
branch
from
c4b29ad
to
416c09a
Compare
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 brings a few benefits:
In this PR I'm also removing the fallback flag to enable in-proc builds; we never needed it and no bugs got filed, so just removing it for good.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1886993