Make Workspaces.MSBuild build with a netstandard target #76832
Conversation
This file contains hidden or 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 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.
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 #71784
ghost
added
Area-IDE
untriaged
JoeRobich
changed the title
CyrusNajmabadi
approved these changes
Jan 21, 2025
JoeRobich
commented
Jan 21, 2025
src/Workspaces/MSBuild/BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Outdated
Show resolved
Hide resolved
JoeRobich
force-pushed
the
dev/jorobich/netstandard-msbuildworkspace
branch
from
3c03719 to
ed80bdf
Compare
src/Workspaces/MSBuild/Core/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj
Show resolved
Hide resolved
| for it. PrivateAssets="all" is needed to prevent this reference from becoming a package reference in the package, as a workaround for | ||
| https://github.com/NuGet/Home/issues/3891. | ||
| --> | ||
| <ProjectReference Include="..\BuildHost\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj" PrivateAssets="all" ReferenceOutputAssembly="false"> |
Member
There was a problem hiding this comment.
I wonder if the lack of a ProjectReference here might have problems in VS understanding there's still an implicit dependency, but maybe it's fine...
src/Workspaces/MSBuild/Test/VisualStudioMSBuildWorkspaceTests.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/MSBuild/BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Show resolved
Hide resolved
src/Workspaces/MSBuild/BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Outdated
Show resolved
Hide resolved
| _process.ErrorDataReceived += Process_ErrorDataReceived; | ||
|
|
||
| var pipeClient = NamedPipeUtil.CreateClient(".", pipeName, PipeDirection.InOut, PipeOptions.Asynchronous); | ||
| // Inlined NamedPipeUtil.CreateClient because it does not support netstandard tfm due to using runtime specific functionality. |
Member
There was a problem hiding this comment.
Which functionality was missing?
Member
Author
There was a problem hiding this comment.
There is not a NETSTANDARD implementation in NamedPipeUtil. We can restore the PipeOptions.CurrentUserOnly for .NET and Mono fairly easily. Adding security for Framework will be more involved.
Member
Author
There was a problem hiding this comment.
Ok. Found the System.IO.Pipes.AccessControl package which makes the security side of this straightforward.
@jaredpar Would there be any interest in fleshing out a NETSTANDARD implementation of NamedPipeUtil?
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.
Reworked #72257
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