Add satellite assemblies to Microsoft.NETCore.Compilers#24353
Add satellite assemblies to Microsoft.NETCore.Compilers#24353tmeschter merged 2 commits intodotnet:dev15.6.xfrom
Conversation
Add satellite assemblies for the Roslyn assemblies that we ship in Microsoft.NETCore.Compiler.nupkg. This includes Microsoft.CodeAnalysis.dll, Microsoft.CodeAnalysis.CSharp.dll, Microsoft.CodeAnalysis.VisualBasic.dll, and Microsoft.Build.Tasks.CodeAnalysis.dll. This increases the resulting .nupkg size from about 8,988KB to about 12,000KB.
🎆|
test windows_release_unit64_prtest please |
|
@Pilchie Is there anything else I need to do (or anyone else I need approval from) before merging this into dev15.6.x? |
|
I would suggest looping @MeiChin-Tsai in so that she is aware, but otherwise I think it can be merged. |
There was an issue in the custom PublishWithoutBuilding target that aims to publish the project without rebuilding anything. It has the same dependencies as the built-in Publish, but replaces build with Just ResolveReferences having BuildProjectReferences=false. However, this caused BuildOnlySettings (a dependency of Build) to not run, and therefore BuildingProject was set to false. That put ResolveAssemblyReferences into design-time mode where FindSatellites became false. This fix is required so that the .deps.json that is generated contains satellite assembly information. Without that, satellites will not load load on .NET Core. There was also a missing dependency on PrepareForPublish, which didn't cause any issue (yet), but it's now added for completeness. Also removed a nearby workaround for an unrelated v1 SDK issue with satellite assemblies. It is no longer needed as Roslyn requires v2+ SDK.
|
I found an issue while testing a private build and pushed 37018e7 with the fix. |
|
@tmeschter @nguerrera @jaredpar Anything holding us back from merging this? |
|
@jasonmalinowski @MeiChin-Tsai has concerns, hence the additional testing by @nguerrera. We need to make sure all of her concerns are addressed. |
|
@nguerrera What else might be affected by the changes you've made to the targets? |
|
A couple questions - given that the size increase, does it impact customer's first use scenario in terms of perf? I assume that this also does not impact CLI inner loop perf, correct? Thanks. |
|
@jaredpar @nguerrera Can you answers MeiChin's questions? I don't have a good sense for exactly how this will be consumed by CLI, or how other end-users may be picking this up. |
The PublishWithoutTesting target is used exclusively for publishing vbc, csc, and VBCSCompiler for .NET Core: https://github.com/dotnet/roslyn/search?utf8=%E2%9C%93&q=PublishWithoutBuilding&type= The workaround just below that I deleted is for a bug that doesn't exist in v2 of the SDK. It is unrelated and could be reverted, but I don't see a real risk.
AFAIK, at this time, the Microsoft.NETCore.Compilers package is only officially supported via its inclusion in the CLI. It has not been release to nuget.org: https://www.nuget.org/packages?q=Microsoft.NETCore.Compilers although I don't know if there are plans to ship it standalone to nuget.org along with 15.6. @jaredpar Can you comment on that? If there is any desire to make this package official on its own and keep its size down, it would totally work to package the satellites in to a separate, parallel Microsoft.NETCore.Compilers.Localization.nupkg...
Correct. This has zero impact on CLI perf. In fact, there is no size impact at all, we're just getting the same *.resources.dll from a much saner process than a separate build in https://github.com/dotnet/cli-deps-satellites. The size on disk for CLI stays exactly the same. There are ways to obtain a CLI without localization (currently, .zip download doesn't have it, .msi does), but those are implemented by simply stripping |
That is correct. This NuGet package is just a vehicle for shipping binaries between the roslyn and cli repos. |
|
@jaredpar @nguerrera @MeiChin-Tsai I'm going to merge this at noon today unless someone yells at me to stop. :-) |
|
I'm fine with this and it's infra only IMHO |
|
@jaredpar @nguerrera This is in now. |
|
@tmeschter 👏 Awesome! Please tag me on the insertion in to CLI that takes this. I shared a commit via email that needs to accompany that. |
|
@nguerrera Who is responsible for the insertions into CLI? |
|
^ @livarcocc did the last one, but I think it usually comes from compiler team. There is an effort to automate it. |
|
I have set it up using maestro, so we should get automatic insertions. Of course, I may have gotten something wrong. |
|
@livarcocc OK, can you keep an eye out for the insertion and tag me when it hits? |
Add satellite assemblies for the Roslyn assemblies that we ship in
Microsoft.NETCore.Compiler.nupkg. This includes
Microsoft.CodeAnalysis.dll, Microsoft.CodeAnalysis.CSharp.dll,
Microsoft.CodeAnalysis.VisualBasic.dll, and
Microsoft.Build.Tasks.CodeAnalysis.dll.
This increases the resulting .nupkg size from about 8,988KB to about
12,000KB.
Customer scenario
This allows consumers of the Microsoft.NETCore.Compilers package to see localized strings when using the tools it contains. However, the immediate motivation for this change is to make it much simpler for the CLI to find and include these localized resources.
Bugs this fixes
N/A
Workarounds, if any
Currently the CLI has a complicated process for locating and pulling in these localized resources.
Risk
Low.
Performance impact
N/A
Is this a regression from a previous update?
N/A
Root cause analysis
N/A
How was the bug found?
N/A
Test documentation updated?
N/A