[BuildHost] Move RPC contracts to a separate assembly #81643
Conversation
dotnet-policy-service
bot
added
VSCode
Needs API Review
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Fine by me, although too bad we don't have a way to avoid all the aliasing hackery. @dibarbet should probably sign off on this too. It's also a bit unfortunate there's no way to (easily) embed this assembly into the other NuGet packages rather than having it be "another" one.
|
Actually come to think of it, we did a trick before to suppress a reference to MSBuild.BuildHost from the "parent" NuGet package; we might be able to do something here. That could be a follow-up though. |
|
Wouldn't a project referencing Workspaces.MSBuild need to reference Workspaces.MSBuild.Contracts package? We can hide BuildHost since it's shipped in Workspaces.MSBuild as content. |
It turned out not too bad imo. extern alias + global using essentially declares we want to import only some namespaces from the given assembly. |
src/Workspaces/MSBuild/Core/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj
Show resolved
Hide resolved
| PackagePath="$([System.IO.Path]::Combine('contentFiles', 'any', 'any', '%(NetFrameworkBuildHostAssets.ContentFolderName)', '%(NetFrameworkBuildHostAssets.TargetPath)'))" | ||
| CopyToOutputDirectory="PreserveNewest" | ||
| PackageCopyToOutput="true" /> | ||
| <Content Include="%(NetFrameworkBuildHostAssets.Identity)" Condition="'%(NetFrameworkBuildHostAssets.TargetPath)' != '' and '%(NetFrameworkBuildHostAssets.Extension)' != '.xml'" TargetPath="$([System.IO.Path]::Combine('%(NetFrameworkBuildHostAssets.ContentFolderName)', '%(NetFrameworkBuildHostAssets.TargetPath)'))" PackagePath="$([System.IO.Path]::Combine('contentFiles', 'any', 'any', '%(NetFrameworkBuildHostAssets.ContentFolderName)', '%(NetFrameworkBuildHostAssets.TargetPath)'))" CopyToOutputDirectory="PreserveNewest" PackageCopyToOutput="true" /> |
There was a problem hiding this comment.
Did you mean to put this on one line?
4 participants
Context: https://github.com/dotnet/roslyn/pull/81577/files?file-filters%5B%5D=.json&file-filters%5B%5D=.cs&file-filters%5B%5D=.txt&file-filters%5B%5D=.csproj#diff-5b690fb7220ca800592067b9ca959ee332ea6455223eab931c526654553aa45c
The extern alias is needed to avoid conflicts between sources linked to BuildHost and Worksapace via MS.CA.Contracts.shproj.
Considered using
Microsoft.CodeAnalysis.EmbeddedAttributebut that has issues - if it's applied on the type that type is not going to be visible in projects (transitively) referencing the project that links the source. Each project would need to reference MS.CA.Contracts.shproj directly and hence we would have N copies of the types. That works for some types, but for not for exchange types likeIReadOnlySet<T>, which need to be the same across all projects that have access to them.VS validation: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=12983270&view=results
https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/696412