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

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

Merged
merged 7 commits into from
Dec 5, 2022

Conversation

dominoFire
Copy link
Contributor

@dominoFire dominoFire commented Sep 16, 2022

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:

<!-- 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

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
install-before install-after install-after-2

nuget.exe Restore

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

Before After
restore-before restore-after restor-after-2

@dominoFire 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 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
@ghost
Copy link

ghost commented Sep 28, 2022

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 dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 8a0651f to dce3d99 Compare September 30, 2022 03:47
@ghost 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 dominoFire marked this pull request as ready for review September 30, 2022 04:45
@dominoFire dominoFire requested a review from a team as a code owner September 30, 2022 04:45
@dominoFire
Copy link
Contributor Author

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

@ghost 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
@ghost
Copy link

ghost commented Oct 12, 2022

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.

@@ -86,6 +86,8 @@
</Compile>
<!-- 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)" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ghost 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 dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from dce3d99 to 47c66e8 Compare October 21, 2022 04:42
@dominoFire dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 388bf42 to e1520db Compare October 21, 2022 22:27
@dominoFire dominoFire marked this pull request as draft October 21, 2022 22:28
@dominoFire dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from e1520db to 0988261 Compare October 22, 2022 00:18
@dominoFire dominoFire requested review from erdembayar, donnie-msft and a team October 22, 2022 00:33
@dominoFire dominoFire marked this pull request as ready for review October 22, 2022 00:34
@dominoFire dominoFire marked this pull request as draft October 22, 2022 00:35
@dominoFire dominoFire force-pushed the dev-dominofire-nugetexe-loc-mixed-embedding branch from 0988261 to d65fd9f Compare October 24, 2022 08:41
@ghost 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
Copy link

ghost commented Oct 31, 2022

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.

@ghost ghost closed this Nov 7, 2022
@dominoFire
Copy link
Contributor Author

Reopening to add functional tests.

@dominoFire dominoFire reopened this Nov 30, 2022
@ghost 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
Removed stderr logging code. Added more condition to intercept nuget.exe failed resources
Co-authored-by: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com>
@dominoFire 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
@dominoFire
Copy link
Contributor Author

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!

@dominoFire dominoFire marked this pull request as ready for review November 30, 2022 10:08
@nkolev92
Copy link
Member

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
Copy link
Contributor Author

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

@dominoFire dominoFire requested review from nkolev92 and a team December 1, 2022 02:33
@dominoFire dominoFire merged commit f2d2e8f into dev Dec 5, 2022
@dominoFire dominoFire deleted the dev-dominofire-nugetexe-loc-mixed-embedding branch December 5, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: nuget.exe strings from NuGet.Commands are not localized
4 participants