Multiple locale support for winget workflows #903

yao-msft · 2021-04-26T21:07:45Z

Change

This change implements support for multi locale in winget workflows.

Exposed locale option in command line as a requirement
Exposed locale option in winget settings as requirement/preference similar to how Scope works
Installer selection uses both locale requirement or preference if applicable
Manifest localization merging(for show) only use locale preference

Validation

Added tests for workflows and ManifestComparato
Tried local run and works as expected

Microsoft Reviewers: Open in CodeFlow

github-actions · 2021-04-26T21:09:12Z

Misspellings found, please review:

bcp
LCIDTo

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"Langs sourc "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"bcp langs LCIDTo "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

github-actions · 2021-04-26T21:12:39Z

Misspellings found, please review:

LCIDTo

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"Langs sourc "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"langs LCIDTo "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

JohnMcPMS · 2021-04-29T23:54:39Z

doc/Settings.md

+
+The `locale` behavior affects the choice of installer based on installer locale. The matching parameter is `--locale`, and uses bcp47 language tag.
+
+Note the locale preference will also be applied to `winget show` command if applicable.


Should they? It feels like a separate setting should exist for display language. #Closed

JohnMcPMS · 2021-04-29T23:57:20Z

src/AppInstallerCLICore/Commands/InstallCommand.cpp

+        {
+            if (!Utility::IsWellFormedBcp47Tag(execArgs.GetArg(Args::Type::Locale)))
+            {
+                throw CommandException(Resource::String::InvalidArgumentValueError, s_ArgumentName_Locale, { "bcp47 language tags"_lis });


s_ArgumentName_Locale

If the Arg is defined in another file, this value should come from finding the Arg and using it's name. #Closed

JohnMcPMS · 2021-04-30T00:01:27Z

src/AppInstallerCLICore/Commands/InstallCommand.cpp

+        {
+            if (!Utility::IsWellFormedBcp47Tag(execArgs.GetArg(Args::Type::Locale)))
+            {
+                throw CommandException(Resource::String::InvalidArgumentValueError, s_ArgumentName_Locale, { "bcp47 language tags"_lis });


"bcp47 language tags"

This is not a localization independent string and it wouldn't fit well into this exception constructor anyway. I think it should just be dropped so the user gets "invalid arg: locale" and can go look at the help on that. #Closed

JohnMcPMS · 2021-04-30T00:04:04Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

-  <data name="LanguageArgumentDescription" xml:space="preserve">
-    <value>Language to install (if supported)</value>
+  <data name="LocaleArgumentDescription" xml:space="preserve">
+    <value>Locale to install (if supported)</value>


Locale to install (if supported)

What about: Locale to use (BCP 47 format) #Closed

JohnMcPMS · 2021-04-30T00:25:52Z

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

+                    if (!preferredList.empty())
+                    {
+                        // TODO: we only take the first one for now
+                        preference = preferredList.at(0);


This suggests that the setting should also be an array. What are the complications with taking the whole list? The only one that comes to mind is that the sort is harder, but I don't think that is insurmountable. #Closed

JohnMcPMS · 2021-04-30T00:28:34Z

src/AppInstallerCommonCore/Public/winget/Locale.h

+#include <string>
+#include <vector>
+
+namespace AppInstaller::Utility


Utility

Locale? #Closed

JohnMcPMS · 2021-04-30T01:32:50Z

src/AppInstallerCommonCore/Manifest/Manifest.cpp

+                if (!preferredList.empty())
+                {
+                    // TODO: we only take the first one for now
+                    targetLocale = preferredList.at(0);


targetLocale = preferredList.at(0);

As discussed, we should use all of these languages and only these for now. #Closed

JohnMcPMS · 2021-04-30T01:49:48Z

src/AppInstallerRepositoryCore/Microsoft/ARPHelper.h

@@ -15,42 +15,42 @@ namespace AppInstaller::Repository::Microsoft
    struct ARPHelper
    {
        // See https://docs.microsoft.com/en-us/windows/win32/msi/uninstall-registry-key for details.
-        std::wstring SubKeyPath{ L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall" };
+        inline static const std::wstring SubKeyPath{ L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall" };


static

Please revert to instance. #Closed

JohnMcPMS · 2021-04-30T21:29:14Z

            result += m_requirement;

is not one of: en-us, es-mx, etc. ?

In reply to: 830410192

Refers to: src/AppInstallerCLICore/Workflows/ManifestComparator.cpp:316 in aa5db9b. [](commit_id = aa5db9b, deletion_comment = True)

JohnMcPMS · 2021-04-30T21:47:22Z

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

+                    double firstScore = first.Locale.empty() ? Locale::UnknownLanguageDistanceScore : Locale::GetDistanceOfLanguage(preferredLocale, first.Locale);
+                    double secondScore = second.Locale.empty() ? Locale::UnknownLanguageDistanceScore : Locale::GetDistanceOfLanguage(preferredLocale, second.Locale);
+
+                    if (firstScore > secondScore && firstScore >= Locale::MinimumDistanceScoreAsCompatibleMatch)


if (firstScore > secondScore && firstScore >= Locale::MinimumDistanceScoreAsCompatibleMatch)

The search through m_preference needs to stop whenever either score is compatible. So:

if (firstScore >= Compat || secondScore => Compat) return firstScore > secondScore;

This is required or you could have IsFirstBetter return true for both orderings of first and second. #Closed

JohnMcPMS

yao-msft added 4 commits April 19, 2021 20:19

Snapshot

f7b029a

8000 Working snapshot

6da2184

Test and tune

41fa793

spelling

2d385e7

yao-msft requested a review from a team as a code owner April 26, 2021 21:07

more spelling

0674cd8

yao-msft added 2 commits April 26, 2021 14:12

Merge remote-tracking branch 'upstream/master' into multilocale

53fcf34

Spelling

aa5db9b

JohnMcPMS reviewed Apr 29, 2021

View reviewed changes

JohnMcPMS reviewed Apr 29, 2021 10000

View reviewed changes

JohnMcPMS reviewed Apr 30, 2021

View reviewed changes

yao-msft added 2 commits April 29, 2021 22:03

PR feedback

b336d46

Merge remote-tracking branch 'upstream/master' into multilocale

174e9b2

JohnMcPMS reviewed Apr 30, 2021

View reviewed changes

yao-msft added 3 commits April 30, 2021 16:16

PR feedback

ed04bc9

minor change

2c6b289

Add more diagnostic logging to track locale selection

85163fd

JohnMcPMS approved these changes May 1, 2021

View reviewed changes

yao-msft merged commit 558f3f4 into microsoft:master May 1, 2021

yao-msft deleted the multilocale branch May 1, 2021 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple locale support for winget workflows #903

Multiple locale support for winget workflows #903


		The `locale` behavior affects the choice of installer based on installer locale. The matching parameter is `--locale`, and uses bcp47 language tag.

		Note the locale preference will also be applied to `winget show` command if applicable.

Multiple locale support for winget workflows #903

Multiple locale support for winget workflows #903

Conversation

Change

Validation

Microsoft Reviewers: Open in CodeFlow

Misspellings found, please review:

Misspellings found, please review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment