-
Notifications
You must be signed in to change notification settings - Fork 690
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
Localizes nuget.exe with default, embedded resource assembly lookup #4773
Conversation
Team Review: Show the before/after for file size and a screenshot showing that strings are now localized |
Update: I found a bug for portuguese language, please hold on review. |
@NuGet/nuget-client Ready for another review. |
e7b9c5e
to
7c08afb
Compare
|
||
if (string.Equals(name.Name, ThisExecutableName, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return typeof(Program).Assembly; | ||
} | ||
// .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") |
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.
Why do we need to special case NuGet.Commands.resources?
I imagine NuGet.resources is the NuGet.Commandline.resources one?
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.
I just followed a comment from that was part of dotnet list|add command implementation. Maybe @rrelyea has some more context here.
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.
Why aren't other assemblies like NuGet.Protocol, NuGet.Configuration, NuGet.Common, etc included as well?
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.
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.
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.
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?
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.
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.
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.
Happy to set up an appointment.
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.
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.
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.
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 :)
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.
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.
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.
Looks good. I added a few clarifying questions so I understand why we're doing what's we're doing.
6972e56
to
29181ea
Compare
|
||
if (string.Equals(name.Name, ThisExecutableName, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return typeof(Program).Assembly; | ||
} | ||
// .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") |
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.
Why aren't other assemblies like NuGet.Protocol, NuGet.Configuration, NuGet.Common, etc included as well?
29181ea
to
fca1565
Compare
@NuGet/nuget-client Ready for another review. |
@dominoFire
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 |
my bad, I missed that, I read the description. So NuGet.Commands strings are not getting localized at all then? |
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. |
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 |
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.
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.
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.
Ok, now it builds twice and ILMerge once
04.BuildNuGetExe.binlog
05.ILMergeNuGetExe.binlog
See reference build
@@ -32,8 +32,8 @@ public class Program | |||
private const string DotNetSetupRegistryKey = @"SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full\"; | |||
private const int Net462ReleasedVersion = 394802; | |||
|
|||
|
|||
private static readonly string ThisExecutableName = typeof(Program).Assembly.GetName().Name; | |||
internal static readonly Assembly NuGetExeAssembly = typeof(Program).Assembly; |
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.
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
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.
I moved all required changes into this PR and left remaining changes for NuGet.Commands in the other PR.
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
88713c7
to
936dc74
Compare
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) ...
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/1771
Regression? Last working version: Somewhere in nuget 3.x
Description
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
Documentation
Validation
Use Case 1:
nuget.exe help
after this PR on Spanish languageRan nuget.exe in Windows 11 with Spanish language:
Use case 2:
nuget.exe help
after this PR on Chinese languageNuGet.exe on Chinese (Traditional) has more localized strings. Console is using code page 65001, changed with command:
chcp 65001
Use case 3:
nuget.exe restore
on a previously restored projectThere are new strings localized. NuGet.Commands string resoures are not not localized in both sides