Make Workspaces.MSBuild build with a netstandard target #72257
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
Now there's MSBuildWorkspace with three folders: - Core (the main library) - BuildHost - Test
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
jasonmalinowski
force-pushed
the
switch-msbuildworkspace-to-netstandard
branch
from
cac3ed1 to
21fc78b
Compare
The tests that need to set things up like this should just be ensuring this state directly, since there's no guarantee that this would have ran before many types of tests run. Fixes dotnet#49486
3 tasks
JoeRobich
reviewed
Feb 26, 2024
| @@ -23,22 +23,22 @@ public RemoteBuildHost(RpcClient client) | |||
| } | |||
|
|
|||
| public Task<bool> HasUsableMSBuildAsync(string projectOrSolutionFilePath, CancellationToken cancellationToken) | |||
| => _client.InvokeAsync<bool>(BuildHostTargetObject, nameof(BuildHost.HasUsableMSBuild), parameters: [projectOrSolutionFilePath], cancellationToken); | |||
| => _client.InvokeAsync<bool>(BuildHostTargetObject, "HasUsableMSBuild", parameters: [projectOrSolutionFilePath], cancellationToken); | |||
There was a problem hiding this comment.
Would it be crazy to add all these methods to a shared interface so that you could continue to use nameof() and ensure the names don't change from beneath you?
We had a few different places doing fallback and this unifies it to a single place.
MSBuildWorkspace had one version that would do the reading in-process but did support solution filters. For VS Code we have one that was doing it out-of-proces and didn't support solution filters. This unifies both approaches, so everybody gets out of process and solution filter support. The solution filter code is simplified around exception handling: there's a bunch of places it'd call TryGetAbsolute*Path asking for it to throw if anything went wrong. We'd then catch that exception and instead rethrow a generic "we couldn't read it" exception that also had no information about the underlying failure. This just simplifies all of that so we'll just let exceptions pass through.
jasonmalinowski
force-pushed
the
switch-msbuildworkspace-to-netstandard
branch
from
21fc78b to
fde57e0
Compare
Before this change we were building the Workspaces.MSBuild library (the part that loads in the end user's application process) as a .NET Core and .NET Framework library with no netstandard target, which meant that if we weren't careful we'd move our .NET Core TFM to something newer than what customers still expect us to support. All of our other libraries target netstandard but this one was still special. This was because some MSBuild NuGet packages themselves don't target netstandard and so we wre forced to do the same. Digging further we realized that Microsoft.Build.Framwork, which defines ILogger was already netstandard compatible, and so our only remaining use of an not-netstandard package was Microsoft.Build, which only existed to read solution files. That I fixed in our prior commit, so at this point the only NuGet packages we still referenced were .NET Standard compatible. Great! There wa one more surprise though: the BuildHost we ship as content files in subdirectories, but we were also shipping the DLL as a regular referenced library in the end user's application. This was to provide the serialization exchange types to the RPC client, as well as share some useful helpers that were needed on both sides. But since the BuildHost still cannot target netstandard because it does need MSBuild libraries that are not yet netstandard, it meant that the regular Workspaces.MSBuild.dll project couldn't reference the BuildHost DLL anymore either. So to break that link I move the handful of files we were needing on both sides to a shared project, and then just include that shared project into both the build host and library/client projects. This means we can break the ProjectReference link entirely. At some point MSBuild will make their other package netstandard, which means that split wasn't strictly necessary to do, but honestly it resulted in some downstream hacks so I believe it's a net win regardless. There was extra MSBuild/NuGet magic to make sure the binary was included in the other project without it appearing as a package reference. The only way to do that was to set PrivateAssets=all, which then meant other projects had to remember to reference that lest we fail to deploy a DLL. It was very much a fight against tooling, and severing the project references just cleans things up nicely. Fixes dotnet#71784
jasonmalinowski
force-pushed
the
switch-msbuildworkspace-to-netstandard
branch
from
fde57e0 to
bffa031
Compare
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.
Before this change we were building the Workspaces.MSBuild library (the part that loads in the end user's application process) as a .NET Core and .NET Framework library with no netstandard target, which meant that if we weren't careful we'd move our .NET Core TFM to something newer than what customers still expect us to support. All of our other libraries target netstandard but this one was still special. This was because some MSBuild NuGet packages themselves don't target netstandard and so we wre forced to do the same.
Digging further we realized that Microsoft.Build.Framework, which defines ILogger was already netstandard compatible, and so our only remaining use of an not-netstandard package was Microsoft.Build, which only existed to read solution files. That I fixed in our prior commit, so at this point the only NuGet packages we still referenced were .NET Standard compatible. Great!
There wa one more surprise though: the BuildHost we ship as content files in subdirectories, but we were also shipping the DLL as a regular referenced library in the end user's application. This was to provide the serialization exchange types to the RPC client, as well as share some useful helpers that were needed on both sides. But since the BuildHost still cannot target netstandard because it does need MSBuild libraries that are not yet netstandard, it meant that the regular Workspaces.MSBuild.dll project couldn't reference the BuildHost DLL anymore either. So to break that link I move the handful of files we were needing on both sides to a shared project, and then just include that shared project into both the build host and library/client projects. This means we can break the ProjectReference link entirely.
At some point MSBuild will make their other package netstandard, which means that split wasn't strictly necessary to do, but honestly it resulted in some downstream hacks so I believe it's a net win regardless. There was extra MSBuild/NuGet magic to make sure the binary was included in the other project without it appearing as a package reference. The only way to do that was to set PrivateAssets=all, which then meant other projects had to remember to reference that lest we fail to deploy a DLL. It was very much a fight against tooling, and severing the project references just cleans things up nicely.
Fixes #71784