Catches failed NuGet.Commands resource events to localize NuGet.Commands with ILMerged nuget.exe resources by dominoFire · Pull Request #4802 · NuGet/NuGet.Client

dominoFire · 2022-09-16T03:24:46Z

Bug

Fixes: NuGet/Home#12097

Regression? Last working version: N/A

Description

This PR embeds nuget.resources assembly into nuget.exe and ILMerge nuget.commands resources, following the idea outline in the comment in nuget.exe project file:

NuGet.Client/src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj

Lines 105 to 107 in be87c9e

    
             <!-- 	Since we are moving some code and strings from NuGet.CommandLine to NuGet.Commands, we opted to go through normal localization process 
        
                   (build .resources.dll) and then add them to the ILMerged nuget.exe --> 
        
             <!-- This will also be called from CI build, after assemblies are localized, since our test infra takes nuget.exe before Localization  -->

Also, this PR adds code to intercept CurrentDomain.ResourceResolve events in nuget.exe when looking for NuGet resources to indicate that NuGet assembly contains the required resource

Over time, nuget.exe file size has increase. This PR represents a 265KB increase in file size, compared to the last nuget.exe version with localization on NuGet.CommandLine and NuGet.Commands assemblies

nuget.exe version	File size in KB	Notes
nuget.3.5.0.exe	4167
nuget.4.9.6.exe	5555
nuget.5.11.3.exe	6633
nuget.6.0.3.exe	6874
nuget.6.1.0.exe	6878
nuget.6.2.0.exe	6926
nuget.6.3.1.exe	6878	NuGet.CommandLine + NuGet.Commands localization
nuget.6.4.0.exe	6299	Introduced nuget.exe localization only for NuGet.CommandLine
This pull request	7143	Adds localization of NuGet.Commands

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception: Removed unit tests since functional test cannot run on localized nuget.exe; Tested manually, see section below
- OR
- N/A
Documentation
- Documentation PR or issue filled: https://github.com/NuGet/docs.microsoft.com-nuget/issues/2949
- OR
- N/A

Validation

Side to side comparison

`nuget.exe` Install

Before: All messages are in English
After: Some messages are translated, some others not because those are strings from nuget.packagemanagement, which are not ILMerged.

Before	After

`nuget.exe` Restore

Before: some messages are localized
After: all restore messages are localized

Before	After

ghost · 2022-09-28T04:01:30Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

dominoFire · 2022-10-05T21:13:59Z

Team Review: Add functional tests (and some sort of environment variable tweaking) to modify OS language and test new added strings.

ghost · 2022-10-12T22:01:32Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

erdembayar · 2022-10-19T16:12:30Z

src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj

    <!-- nuget.exe localized assemblies -->
    <EmbeddedResource Include="$(OutputPath)\**\$(AssemblyName).resources.dll" Exclude="@(MergeExclude)" />
+    <!-- NuGet.Commands localized assemblies -->
+    <LocResource Include="$(OutputPath)\**\NuGet.Commands.resources.dll" Exclude="@(MergeExclude)" />


I'm just curious: what is the difference between this one and string resourceName = $"NuGet.CommandLine.{culture.Name}.{name}.dll"; in line 269? Which one contains exactly which resources?

Below is a comparison table.

Name Description How it's shipped in nuget.exe

Nuget.CommandLine.{culture.Name}.{name}.dll This is the name of a DLL embedded as a resource. Since we rename NuGet.CommandLine.exe to NuGet.exe, {name} part becomes "NuGet.Resources" Embedded in NuGet.exe

NuGet.Commands.resources.dll Contains all strings from NuGet.Commands project ILMerged

LMK if you have more questions.

src/NuGet.Clients/NuGet.CommandLine/Program.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs

src/NuGet.Core/NuGet.Commands/CLILanguageOverrider.cs

src/NuGet.Core/NuGet.Commands/LanguageEnvironmentVariable.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetHelpCommandTest.cs

ghost · 2022-10-31T10:01:38Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

dominoFire · 2022-11-30T03:08:39Z

Reopening to add functional tests.

Removed stderr logging code. Added more condition to intercept nuget.exe failed resources

typo

Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com>

…cenario. Updated project file due to rebase (CPM repo onboarding)

dominoFire · 2022-11-30T10:07:43Z

Another approach would be to not merge this PR and find a workaround to use NuGet.Localization package to localize nuget.exe ; see https://github.com/NuGet/docs.microsoft.com-nuget/issues/2949

If you like this idea, please upvote this comment. Thanks!

nkolev92 · 2022-11-30T18:52:38Z

Wouldn't not merging this PR regress the experience for nuget.commands strings prior to us starting the work to update localization?

IMO we should try to make this PR work and then listen for feedback.

dominoFire · 2022-11-30T22:51:54Z

Wouldn't not merging this PR regress the experience for nuget.commands strings prior to us starting the work to update localization?

IMO we should try to make this PR work and then listen for feedback.

Ok, then, let's review this PR @NuGet/nuget-client

src/NuGet.Clients/NuGet.CommandLine/Program.cs

dominoFire mentioned this pull request Sep 16, 2022

Localizes nuget.exe with default, embedded resource assembly lookup #4773

Merged

8 tasks

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch 2 times, most recently from a65d97d to 8a0651f Compare September 21, 2022 01:19

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 28, 2022

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 8a0651f to dce3d99 Compare September 30, 2022 03:47

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 30, 2022

dominoFire marked this pull request as ready for review September 30, 2022 04:45

dominoFire requested a review from a team as a code owner September 30, 2022 04:45

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 12, 2022

erdembayar reviewed Oct 19, 2022

View reviewed changes

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 19, 2022

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from dce3d99 to 47c66e8 Compare October 21, 2022 04:42

donnie-msft reviewed Oct 21, 2022

View reviewed changes

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 388bf42 to e1520db Compare October 21, 2022 22:27

dominoFire marked this pull request as draft October 21, 2022 22:28

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from e1520db to 0988261 Compare October 22, 2022 00:18

dominoFire requested review from a team, donnie-msft and erdembayar October 22, 2022 00:33

dominoFire marked this pull request as ready for review October 22, 2022 00:34

dominoFire marked this pull request as draft October 22, 2022 00:35

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 0988261 to d65fd9f Compare October 24, 2022 08:41

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 31, 2022

ghost closed this Nov 7, 2022

dominoFire reopened this Nov 30, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Nov 30, 2022

Fernando Aguilar and others added 4 commits November 29, 2022 21:05

Mixing embedding types

6fa43c4

Removed stderr logging code. Added more condition to intercept nuget.exe failed resources

Fixed rebase

4e38bbe

Test: Added 1 test for spanish language

a6b1c59

typo

Apply suggestions from code review

cb6d3c4

Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com>

dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from d65fd9f to cb6d3c4 Compare November 30, 2022 05:21

Fernando Aguilar added 3 commits November 30, 2022 01:41

Removed test since UILanguageOverrideTests covers language override s…

e06987f

…cenario. Updated project file due to rebase (CPM repo onboarding)

Removed unneeded changes

c0e0186

min-changeset

aac22bf

dominoFire marked this pull request as ready for review November 30, 2022 10:08

nkolev92 reviewed Nov 30, 2022

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/Program.cs Show resolved Hide resolved

dominoFire requested review from a team and nkolev92 December 1, 2022 02:33

nkolev92 approved these changes Dec 1, 2022

View reviewed changes

dominoFire merged commit f2d2e8f into dev Dec 5, 2022

dominoFire deleted the dev-dominofire-nugetexe-loc-mixed-embedding branch December 5, 2022 20:06

	<!-- Since we are moving some code and strings from NuGet.CommandLine to NuGet.Commands, we opted to go through normal localization process
	(build .resources.dll) and then add them to the ILMerged nuget.exe -->
	<!-- This will also be called from CI build, after assemblies are localized, since our test infra takes nuget.exe before Localization -->

Name	Description	How it's shipped in `nuget.exe`
`Nuget.CommandLine.{culture.Name}.{name}.dll`	This is the name of a DLL embedded as a resource. Since we rename NuGet.CommandLine.exe to NuGet.exe, {name} part becomes "NuGet.Resources"	Embedded in NuGet.exe
`NuGet.Commands.resources.dll`	Contains all strings from NuGet.Commands project	ILMerged

Conversation

dominoFire commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Validation

nuget.exe Install

nuget.exe Restore

Uh oh!

ghost commented Sep 28, 2022

Uh oh!

dominoFire commented Oct 5, 2022

Uh oh!

ghost commented Oct 12, 2022

Uh oh!

erdembayar Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

dominoFire Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Oct 31, 2022

Uh oh!

dominoFire commented Nov 30, 2022

Uh oh!

dominoFire commented Nov 30, 2022

Uh oh!

nkolev92 commented Nov 30, 2022

Uh oh!

dominoFire commented Nov 30, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dominoFire commented Sep 16, 2022 •

edited

Loading

`nuget.exe` Install

`nuget.exe` Restore