[pve-devel] [PATCH v2 installer 1/1] Fix #1527: Use 'iso-codes' package country naming

[Thomas Lamprecht]

On 9/13/18 3:45 PM, Rhonda D'Vine wrote:
> The iso 3166-1 information in the iso-codes package seems to be more
> up to par than the one that was used from tzdata.
> 
> Signed-off-by: Rhonda D'Vine <rhonda at proxmox.com>
> ---

looks OK in general, few nits still inline. Oh, and for single patches
a cover-letter is not required, you can write your changes here in this
area: below the triple dash and before the stats start, just FYI.

>  country.pl     | 14 ++++----------
>  debian/control |  1 +
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/country.pl b/country.pl
> index 277fc34..ea75908 100755
> --- a/country.pl
> +++ b/country.pl
> @@ -4,24 +4,18 @@ use strict;
>  use warnings;
>  
>  use PVE::Tools;
> +use JSON;
>  
>  # see also: http://en.wikipedia.org/wiki/Keyboard_layout
>  #
> -# country codes from: /usr/share/zoneinfo/iso3166.tab
> +# country codes from: /usr/share/iso-codes/json/iso_3166-1.json

thanks - and you're right, the rest is quite obsolete.
I have still a nit below, maybe you can address it in a v3 and remove also
the obsolete comments.

>  # timezones from: /usr/share/zoneinfo/zone.tab
>  # keymaps: find /usr/share/keymaps/i386/ -type f -name '*.kmap.gz'
>  # x11 layouts: /usr/share/X11/xkb/rules/xorg.lst
>  
> -my $country = {};
> +my $iso_3166_codes = from_json(PVE::Tools::file_get_contents('/usr/share/iso-codes/json/iso_3166-1.json', 40000));

I'd like to have the path pulled out in it's own variable, you
could put it above where the comment block is (was). Forgot to
mention that previously, sorry about that.

The size would be nice to have in (X * 1024) format a reader quickly
sees that it's X kilobyte, without the need of counting zeros.
And while the file is now ~ 36 KB big, and your 40 KB may be enough,
I'd just give it a 64 or 128 * 1024 size, power of two just have
something nice for being magic numbers :)
I can also do those changes in a followup commit, if you prefer that.

>  
> -my $line;
> -open (TMP, "</usr/share/zoneinfo/iso3166.tab");
> -while (defined ($line = <TMP>)) {
> -    if ($line =~ m/^([A-Z][A-Z])\s+(.*\S)\s*$/) {
> -	$country->{lc($1)} = $2;
> -    }
> -}
> -close (TMP);
> +my $country = { map { lc($_->{'alpha_2'}) => $_->{'common_name'} // $_->{'name'} } @{$iso_3166_codes->{'3166-1'}} };
>  
>  # we need mappings for X11, console, and kvm vnc
>  
> diff --git a/debian/control b/debian/control
> index 60bc8b0..7ab4515 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -3,6 +3,7 @@ Section: perl
>  Priority: optional
>  Maintainer: Proxmox Support Team <support at proxmox.com>
>  Build-Depends: debhelper (>= 9),
> +               iso-codes,
>                 libpve-common-perl,
>                 librsvg2-bin,
>                 perl (>= 5.10.0-19),
>