Skip to content
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

NuGet.Localization package includes unncessary resource assemblies #11342

Closed
pranavkm opened this issue Oct 28, 2021 · 9 comments · Fixed by NuGet/NuGet.Client#5780
Closed

NuGet.Localization package includes unncessary resource assemblies #11342

pranavkm opened this issue Oct 28, 2021 · 9 comments · Fixed by NuGet/NuGet.Client#5780
Assignees
Labels
Functionality:SDK The NuGet client packages published to nuget.org Partner:CLI-SDK Priority:2 Issues for the current backlog. Tenet:WorldReady Type:DCR Design Change Request

Comments

@pranavkm
Copy link
Contributor

NuGet Product(s) Affected

dotnet.exe

Current Behavior

The NuGet.Localization assembly is extracted as-is by the .NET SDK as part of creating it's layout. It looks like the package contains resources for the NuGet.VisualStudio.Contracts.dll but not the primary dll itself. Here's what I get if I list the contents of the .NET 6 SDK:

> ls -r d:\temp\dotnet6\sdk *NuGet.VisualStudio.Contracts*.dll | Select FullName

FullName
--------
D:\temp\dotnet6\sdk\6.0.100\cs\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\de\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\es\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\fr\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\it\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\ja\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\ko\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\pl\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\pt-BR\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\ru\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\tr\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\zh-Hans\NuGet.VisualStudio.Contracts.resources.dll
D:\temp\dotnet6\sdk\6.0.100\zh-Hant\NuGet.VisualStudio.Contracts.resources.dll

Desired Behavior

I'm not certain, but it looked like the localization package is only consumed by the SDK. In which case the assembly could be dropped from the package, helping reduce the total size of the installer. Alternatively, we'd have consider doing something in the SDK to trim the assembly as part of consuming this package.

Additional Context

No response

@pranavkm pranavkm added Triage:Untriaged Type:DCR Design Change Request labels Oct 28, 2021
@AraHaan
Copy link

AraHaan commented Nov 3, 2021

hmm perhaps there should be a node like: <ExcludeResourceLanguages></ExcludeResourceLanguages> added to <PackageReference> that perhaps might solve this issue? I guess an msbuild update for it could help with this one instead of changing the NuGet.Localization package for everyone.

@heng-liu
Copy link
Contributor

Hi @dominoFire , could you help take a look? Thanks!

@dominoFire
Copy link
Contributor

We can consider removing NuGet.VisualStudio.Contracts resource assemblies from NuGet.Localization package, and those without string resrouces. Table below shows NuGet.Localization assembly contents.

NuGet.PackageManagement is designed to work with VS, whose localization ships inside VS. That requires further analysis.

Resource DLL Is VS specific? Contains String Resources?
Microsoft.Build.NuGetSdkResolver No Yes
NuGet.Build.Tasks.Console No No
NuGet.Build.Tasks No Yes
NuGet.CommandLine.XPlat No Yes
NuGet.Commands No Yes
NuGet.Common No Yes
NuGet.Configuration No Yes
NuGet.Credentials No Yes
NuGet.DependencyResolver.Core No Yes
NuGet.Frameworks No Yes
NuGet.LibraryModel No Yes
NuGet.Localization No No
NuGet.PackageManagement Yes Yes
NuGet.Packaging.Core No No
NuGet.Packaging No Yes
NuGet.ProjectModel No Yes
NuGet.Protocol No Yes
NuGet.Resolver No Yes
NuGet.Versioning No Yes
NuGet.VisualStudio.Contracts Yes No

@pranavkm Please tell us which assembly resources are needed in dotnet SDK. Thanks!

@dominoFire dominoFire added WaitingForCustomer Applied when a NuGet triage person needs more info from the OP Tenet:WorldReady Triage:Discussions and removed Triage:Untriaged labels Nov 16, 2021
@AraHaan
Copy link

AraHaan commented Nov 16, 2021

I think a much better option is to be able to tell the .NET SDK to not include to resource assemblies by adding support to msbuild for:

<ExcludeResourceAssemblies>true</ExcludeResourceAssemblies>

on project/package references.

@nkolev92
Copy link
Member

nkolev92 commented Nov 23, 2021

@pranavkm Please tell us which assembly resources are needed in dotnet SDK. Thanks!

@dominoFire

We know which assembly resources are needed in the .NET SDK, it's the ones that we insert.

We can clean up the authoring of the NuGet.Localization package, https://github.com/NuGet/NuGet.Client/blob/a8b6b1fb3a1bccd207a83612cd0f5e1f4ff1d0b8/src/NuGet.Core/NuGet.Localization/NuGet.Localization.nuspec#L21.

Alternatively, the installer could probably exclude certain files if necessary?
I know there is special authoring for packages such as the NuGet.Build.Tasks.Pack for example.

@nkolev92 nkolev92 added Partner:CLI-SDK Functionality:SDK The NuGet client packages published to nuget.org and removed Triage:Discussions WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Nov 23, 2021
@zivkan
Copy link
Member

zivkan commented Nov 23, 2021

@nkolev92 do you have historical context for why NuGet doesn't ship translations in their respective packages, rather than this NuGet.Localization package? Is package size the only concern? My gut feeling is that this package shouldn't even exist.

@nkolev92
Copy link
Member

I don't have really have all the context.

Package size was probably a pretty big concern.

Localized packages are not the smoothest experience.
You can put them in a package, but those assemblies getting consumed the right way is not easy in all scenarios.

I think there's a do customers want it type of consideration as well.

@zivkan
Copy link
Member

zivkan commented Nov 29, 2021

From @dominoFire's table, NuGet.Localization.resources.dll can be removed, since there is no assembly/code (it's not used either by the .NET SDK or by VS)

@zivkan zivkan added the Priority:2 Issues for the current backlog. label Nov 29, 2021
@AraHaan
Copy link

AraHaan commented Nov 30, 2021

Hmm I wonder why there cant be a way to have all of the resources from all of the assemblies into a single central assembly.

Like NuGet.Resources (where only nuget's packages can see the code that loads the resources as they will all be "internal") that is then used by all of them, and makes everything a lot easier to have it as well. This would then ensure that the .NET SDK, and VS would not contain needless resource assemblies I think as well (Bonus points if trimming also would trim unused resources as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:SDK The NuGet client packages published to nuget.org Partner:CLI-SDK Priority:2 Issues for the current backlog. Tenet:WorldReady Type:DCR Design Change Request
Projects
None yet
6 participants