Localizes nuget.exe with default, embedded resource assembly lookup by dominoFire · Pull Request #4773 · NuGet/NuGet.Client

dominoFire · 2022-08-30T02:24:38Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1771

Regression? Last working version: Somewhere in nuget 3.x

Description

nuget.exe now has localized strings from localization team
Localized resources are now embedded into nuget.exe
A custom resource lookup is implemented in assembly lookup event: CurrentDomain_AssemblyResolve
If a string is not found, the current default resource lookup mechanism (a string prefixed with three-letter language name) is used as fallback
Removed duplicated translated strings in NuGetResources.resx, NuGetCommand.resx; those duplicated strings were removed using a custom program to identify those strings. More info at: Adds tool to lock translated strings in nuget.exe Entropy#90

NOTE: To ease reviewing process, it is encouraged to skip changes in .Designer.cs and .resx files; instead, review the program to identify and remove duplicated strings, linked in Entropy PR.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added: Added unit tests
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled: Maybe, we will require a doc issue documenting the resource lookup for nuget.exe and how this affects nuget.exe extensions. It's expected to fallback into current method.
- OR
- N/A

Validation

Use Case 1: `nuget.exe help` after this PR on Spanish language

Ran nuget.exe in Windows 11 with Spanish language:

Use case 2: `nuget.exe help` after this PR on Chinese language

NuGet.exe on Chinese (Traditional) has more localized strings. Console is using code page 65001, changed with command: chcp 65001

Before	After

Use case 3: `nuget.exe restore` on a previously restored project

There are new strings localized. NuGet.Commands string resoures are not not localized in both sides

Before	After

jeffkl · 2022-08-30T17:27:53Z

Team Review: Show the before/after for file size and a screenshot showing that strings are now localized

eng/pipelines/templates/Build_and_UnitTest.yml

dominoFire · 2022-08-31T00:16:41Z

Update: I found a bug for portuguese language, please hold on review.

dominoFire · 2022-08-31T03:15:47Z

@NuGet/nuget-client Ready for another review.

nkolev92 · 2022-09-02T00:49:18Z

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

            }
+            // .NET Framework 4.x now triggers AssemblyResolve event for resource assemblies
+            // Catch this event for nuget.exe (NuGet.CommandLine) and NuGet.Command resource assemblies only
+            if (name.Name == "NuGet.resources" || name.Name == "NuGet.Commands.resources")


Why do we need to special case NuGet.Commands.resources?

I imagine NuGet.resources is the NuGet.Commandline.resources one?

I just followed a comment from that was part of dotnet list|add command implementation. Maybe @rrelyea has some more context here.

Why aren't other assemblies like NuGet.Protocol, NuGet.Configuration, NuGet.Common, etc included as well?

Because if we include all NuGet assemblies, this would result in a larger nuget.exe binary, 13 MB approximately.

Also, I think some core localization assemblies are shipped in NuGet.Localization package. Customers can use that package if they want.

When a customer's machine uses a system language other than English, how can a customer using nuget.exe use NuGet.Localization to "fix" messages that are being output in English?

My point is that if you want to tell customers to use NuGet.Localization then that's a UX change. We're adding a feature, but we don't have details on how they can use it.

Happy to set up an appointment.

Just to clarify my comment.

If you want to recommend that, then we need a spec.

What prompted the whole discussion was the fact that it isn't obvious that we're only localizing nuget.commands and nuget.commandline only.
The current implementation doesn't really localize strings from nuget.commands correctly, am that right?

NuGet.Protocol contains a healthy amount of strings that are aimed at the user and it's something that restore will frequently do.
We should consider adding nuget.protocol as well imo.

The current implementation doesn't really localize strings from nuget.commands correctly, am that right?

That's correct.

We should consider adding nuget.protocol as well imo.

We can do that in another PR :)

If you want to recommend that, then we need a spec.

I am going to enable support for nuget.exe localization; I just tried this approach of placing the satellite assemblies alongside nuget.exe and it didn't work. So, not recommended.

eng/pipelines/templates/Build_and_UnitTest.yml

nkolev92

Looks good. I added a few clarifying questions so I understand why we're doing what's we're doing.

eng/pipelines/templates/Build_and_UnitTest.yml

zivkan · 2022-09-05T20:46:11Z

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

            }
+            // .NET Framework 4.x now triggers AssemblyResolve event for resource assemblies
+            // Catch this event for nuget.exe (NuGet.CommandLine) and NuGet.Command resource assemblies only
+            if (name.Name == "NuGet.resources" || name.Name == "NuGet.Commands.resources")


Why aren't other assemblies like NuGet.Protocol, NuGet.Configuration, NuGet.Common, etc included as well?

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

dominoFire · 2022-09-16T02:41:50Z

@NuGet/nuget-client Ready for another review.

nkolev92 · 2022-09-16T20:31:46Z

@dominoFire
One of the follow ups from the previous discussion was:

Fernando will add before/after comparisons to see if NuGet.Commands is localized before this change. Focus on nuget.exe restore examples.

Did you try that?

dominoFire · 2022-09-16T20:39:08Z

@dominoFire
One of the follow ups from the previous discussion was:

Fernando will add before/after comparisons to see if NuGet.Commands is localized before this change. Focus on nuget.exe restore examples.

Did you try that?

Yeah, it's on the description. It's use case number 3. The 'assets file has not changed' is a string from NuGet.Commands

nkolev92 · 2022-09-16T21:22:02Z

my bad, I missed that, I read the description.

So NuGet.Commands strings are not getting localized at all then?
Shouldn't the assembly resolver logic have ensure that it'd get localized in that case?

dominoFire · 2022-09-16T22:36:15Z

my bad, I missed that, I read the description.

So NuGet.Commands strings are not getting localized at all then? Shouldn't the assembly resolver logic have ensure that it'd get localized in that case?

Yes! I created a follow-up PR for that: #4802

I just want to have this PR merged to continue on that.

Think of this PR as the work needed to localize nuget.exe

Thinkf of the second PR as the work to fix NuGet.Commands localization.

nkolev92 · 2022-09-16T23:19:13Z

eng/pipelines/templates/Build_and_UnitTest.yml

-    msbuildArguments: "/t:ILMergeNuGetExe /p:ExpectedLocalizedArtifactCount=$(LocalizedLanguageCount) /bl:$(Build.StagingDirectory)\\binlog\\04.ILMergeNuGetExe.binlog"
+    msbuildArguments: "/t:Build /bl:$(Build.StagingDirectory)\\binlog\\04.BuildNuGetExe.binlog"
+
+- task: MSBuild@1


If you look at the logs for Build NuGet.exe Localized, you will see that we're now ilmerging twice.
We need to build twice, but no need to ilmerge twice.

Ok, now it builds twice and ILMerge once

04.BuildNuGetExe.binlog

05.ILMergeNuGetExe.binlog

See reference build

nkolev92 · 2022-09-16T23:24:00Z

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


-
-        private static readonly string ThisExecutableName = typeof(Program).Assembly.GetName().Name;
+        internal static readonly Assembly NuGetExeAssembly = typeof(Program).Assembly;


Given that these changes are not going to make any localization work, should we just delayed that for the future PR?

It seems like the new branch you have mostly undoes these, dev-dominofire-nugetexe-defaultloc...dev-dominofire-nugetexe-loc-mixed-embedding

I moved all required changes into this PR and left remaining changes for NuGet.Commands in the other PR.

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

parent 95be29c author Fernando Aguilar <feaguila@microsoft.com> 1659511997 -0500 committer Fernando Aguilar <feaguila@microsoft.com> 1662080451 -0700 Use default resource manager for string resources in nuget.exe ***NO_CI*** Remove whitespace Fixed bug on unreachable strings

Tests: Added tests Tests: modified tests Fixed tests Fixed more tests

Trying to fix loc CI pipeline Removed unused build variable

Tests: added tests for internal method Tests: Skip unsupported locales on mono Tests: Added one more test to windows-only locale test

- Building twice, ilmerging once - Removed code not to be used in second PR

dominoFire · 2022-09-20T22:37:23Z

Just found out that this breaks nuget.mssigning.exe localization. Investigating.

Insert 6.4.0-rc.123 into rel/d17.4 on 11/07/2022 23:47:12 * tag '6.4.0.123': (60 commits) fix a logic error that caused AbandonedMutexException while executing migrations (release-6.4.x) (NuGet#4895) unblock source build failing due to fatal: transport 'file' not allowed error (NuGet#4867) (NuGet#4874) Signing: update to August 2022 CTL (NuGet#4791) (NuGet#4850) Merged PR 422933: Prefer BCL Directory create API over helper class (7.0.1xx-rc2) Fix empty combobox when package is not present in project file (NuGet#4844) (NuGet#4848) Fix component detection alert for microsoft.owin package (NuGet#4841) (NuGet#4845) Make release label RC, move to escrow mode Adds special case to include transitive origins in GetInstalledAndTransitivePackagesAsync API (NuGet#4824) Add longPathAware manifest to NuGet.Build.Tasks.Console (NuGet#4830) VsPackageInstallerServices should not post ProjectNotNominatedException faults (NuGet#4814) Skip test GetOrCreateAsync_WithUnhandledExceptionInPlugin_Throws (NuGet#4831) Improve OptProf pipeline job run names (NuGet#4825) Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio (NuGet#4798) Suppress CA2213 warnings to unblock dev branch (NuGet#4823) Ensure IsVsOfflineFeed is calculated correctly on 64-bit machines (NuGet#4817) Add better handling of AggregateExceptions in static graph-based restore (NuGet#4809) Add Component Detection task into each pipeline (NuGet#4813) Localizes nuget.exe with default, embedded resource assembly lookup (NuGet#4773) Removes BrowseObjectBase class in NuGet Solution Explorer (NuGet#4807) Improve TryCreateContext (NuGet#4762) ...

dominoFire requested a review from a team as a code owner August 30, 2022 02:24

nkolev92 reviewed Aug 30, 2022

View reviewed changes

eng/pipelines/templates/Build_and_UnitTest.yml Outdated Show resolved Hide resolved

dominoFire requested review from a team and nkolev92 August 31, 2022 00:16

dominoFire force-pushed the dev-dominofire-nugetexe-defaultloc branch from e7b9c5e to 7c08afb Compare August 31, 2022 03:19

dominoFire mentioned this pull request Aug 31, 2022

Removes unused string resources in nuget.exe #4779

Merged

8 tasks

nkolev92 reviewed Sep 2, 2022

View reviewed changes

eng/pipelines/templates/Build_and_UnitTest.yml Outdated Show resolved Hide resolved

dominoFire force-pushed the dev-dominofire-nugetexe-defaultloc branch from 6972e56 to 29181ea Compare September 2, 2022 09:22

dominoFire requested review from a team, nkolev92 and zivkan September 2, 2022 22:24

zivkan reviewed Sep 5, 2022

View reviewed changes

dominoFire force-pushed the dev-dominofire-nugetexe-defaultloc branch from 29181ea to fca1565 Compare September 7, 2022 02:50

dominoFire requested review from a team and zivkan September 7, 2022 18:54

dominoFire mentioned this pull request Sep 16, 2022

Catches failed NuGet.Commands resource events to localize NuGet.Commands with ILMerged nuget.exe resources #4802

Merged

8 tasks

nkolev92 reviewed Sep 16, 2022

View reviewed changes

Fernando Aguilar added 4 commits September 19, 2022 19:59

Adding NuGet.CommandLine resources to merged nuget.exe

21ac5d7

Adding more loc lookup code

046598a

Used new variable syntax for pipeline variables

08688c1

Fernando Aguilar added 18 commits September 19, 2022 19:59

Included all localized assemblies in nuget.exe

2137e49

Hardcoding localized assembly count

492a509

Only merging Nuget.exe and NuGet.Commands.dll localized resources

da5ad1b

Getting resource assemblies from embedded resources

f03f409

Added license to source file

8957188

Min-changeset

a66da18

Tests: More tests

a97d8ed

Tests: Added tests Tests: modified tests Fixed tests Fixed more tests

Pipeline mods

0c205bf

Trying to fix loc CI pipeline Removed unused build variable

Fixed case for cultures with dashes

bed4471

Tests: Added test

8ac065a

Tests: added tests for internal method Tests: Skip unsupported locales on mono Tests: Added one more test to windows-only locale test

Second build to include embedded localized resources

ee48733

Replaced hardcoded string with NuGetResources.ArgumentNullOrEmpty

e870b58

Removed unneeded localized nuget.exe string resources

26d0a84

Caching executing assembly

4747afd

Tests: Forced English output to fix tests

714d0d8

Separating build and ilmerge steps

f9bb656

Removing test changes. Assuming CI with english language

b19357b

Feedback

936dc74

- Building twice, ilmerging once - Removed code not to be used in second PR

dominoFire force-pushed the dev-dominofire-nugetexe-defaultloc branch from 88713c7 to 936dc74 Compare September 20, 2022 03:00

dominoFire requested review from a team and nkolev92 September 20, 2022 03:30

nkolev92 approved these changes Sep 20, 2022

View reviewed changes

dominoFire requested a review from a team September 20, 2022 20:17

nkolev92 approved these changes Sep 20, 2022

View reviewed changes

dominoFire merged commit a6210b4 into dev Sep 21, 2022

dominoFire deleted the dev-dominofire-nugetexe-defaultloc branch September 21, 2022 00:52



		private static readonly string ThisExecutableName = typeof(Program).Assembly.GetName().Name;
		internal static readonly Assembly NuGetExeAssembly = typeof(Program).Assembly;

Conversation

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

Uh oh!

Bug

Description

PR Checklist

Validation

Use Case 1: nuget.exe help after this PR on Spanish language

Use case 2: nuget.exe help after this PR on Chinese language

Use case 3: nuget.exe restore on a previously restored project

Uh oh!

jeffkl commented Aug 30, 2022

Uh oh!

Uh oh!

dominoFire commented Aug 31, 2022

Uh oh!

dominoFire commented Aug 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dominoFire commented Sep 16, 2022

Uh oh!

nkolev92 commented Sep 16, 2022

Uh oh!

dominoFire commented Sep 16, 2022

Uh oh!

nkolev92 commented Sep 16, 2022

Uh oh!

dominoFire commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dominoFire commented Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dominoFire commented Aug 30, 2022 •

edited

Loading

Use Case 1: `nuget.exe help` after this PR on Spanish language

Use case 2: `nuget.exe help` after this PR on Chinese language

Use case 3: `nuget.exe restore` on a previously restored project

dominoFire Sep 6, 2022 •

edited

Loading

nkolev92 Sep 7, 2022 •

edited

Loading

dominoFire Sep 20, 2022 •

edited

Loading