-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[release/10.0] Copy BuildHost instead of using contentFiles from Microsoft.CodeAnalysis.Workspaces.MSBuild #37114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6c0b0aa to
6faf008
Compare
bad4051 to
8b0896e
Compare
8b0896e to
b75d892
Compare
b75d892 to
f158740
Compare
|
@AndriySvyryd code complete for 10.0.101 was two days ago but we don't have a final build. If this absolutely must to go into 101, please send out a mail to Tactics asap. |
This is not needed for 10.0.101 - can go into the next servicing release |
c11a063 to
4ded0cf
Compare
|
cc @artl93 - this is ready for your approval prior to Tactics |
|
@AndriySvyryd - the change is very large. If it's targeted, why is it this large? What validation has been done to assure that we've got this nailed? |
|
Most of the changes are in test baselines caused by the upgrade of |
artl93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Understood about why the change is so big. When you send the mail to tactics, definitely include the variety of things tested. If we have to take this all in one shot anyway to have a complete fix, breaking this up doesn't help much.
Based on scenario, coverage, regression, customer-reported, I approve. Make sure the customer scenario (what the customer does and sees) is clear when submitting to tactics. Why the build host folder appears in the output folder as being a problem I don't think will be apparent to the reviewers. The screenshot in the original issue does a pretty good job - consider including that.
7e15efd to
0e80178
Compare
…osoft.CodeAnalysis.Workspaces.MSBuild Fixes #36970
0e80178 to
82468ab
Compare
Fixes #36970
Description
Microsoft.CodeAnalysis.Workspaces.MSBuildrequires BuildHost to be available in the output folder for projects that copy dependencies to the output folder. The previous fix removed contentFiles from PrivateAssets of theMicrosoft.CodeAnalysis.Workspaces.MSBuildreference to achieve this, however it has a side effect of also copying BuildHost to the publish folder. A more targeted fix is to copy over BuildHost to output folder only when needed.Microsoft.CodeAnalysis.Workspaces.MSBuild5.0.0 needs to be referenced as it contains a fix to load BuildHost from the NuGet cache in cases where dependencies aren't copied to the output folder. Most of the changes are in test baselines caused by slight differences in the code produced byMicrosoft.CodeAnalysis.*Customer impact
For project referencing
Microsoft.EntityFrameworkCore.Design(the vast majority of projects using EF) BuildHost folders appear in the solution and get copied to the publish folder, significantly increasing the total size. The workaround is to delete them or to add contentFiles to PrivateAssets for projects that don't use BuildHost.How found
Customer reported on 10.0.0-rc2.
Regression
Yes, from 10.0.0-rc1. Introduced in #36708
Testing
Validated the fix manually by using EF tools on a variety of projects:
Risk
Low, only design-time tools are affected by the change.