Re: [PATCH] localedata: Reorganize Unicode LC_CTYPE inclusion.

[Carlos O'Donell <carlos at redhat dot com>]

On 10/23/2017 03:21 AM, Rafal Luzynski wrote:
> Sorry for the late review but here it goes.  I've found few minor
> nitpicks and one major problem.

Thank you!
 
> 13.10.2017 08:49 Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> [...]
>> (i18n-report): Rename to...
>> (i18n_ctype-report): ...this.
> 
> Was this change intentional?  This is just the name of the report file
> generated by make check.  Technically it does not matter what is its
> name.  You didn't have to change it but it's OK if you have decided
> to do this.

It was intentional.

The file is now `i18n_ctype` so I wanted the report to reflect that it
was producing a report for that file.

>> diff --git a/localedata/locales/i18n_ctype b/localedata/locales/i18n_ctype
>> new file mode 100644
>> index 0000000..da88efc
>> --- /dev/null
>> +++ b/localedata/locales/i18n_ctype
>> @@ -0,0 +1,2343 @@
>> +escape_char /
>> +comment_char %
>> +
>> +% This file is part of the GNU C Library and contains locale data.
>> +% The Free Software Foundation does not claim any copyright interest
>> +% in the locale data contained in this file.  The foregoing does not
>> +% affect the license of the GNU C Library as a whole.  It does not
>> +% exempt you from the conditions of the license if your use would
>> +% otherwise be governed by that license.
>> +
>> +%
>> +% The only purpose for this file is to provide the basic Unicode
>> +% LC_CTYPE information.  This way other locales that need this
>> +% information, but with different transliterations, can include it
>> +% directly.
> 
> This is probably a minor problem but I've tried to redo your work
> and in my regenerated file I've received this single line instead:
> 
> % Generated automatically by gen_unicode_ctype.py for Unicode 10.0.0.
> 
> My guess is that you've first copied localedata/locales/i18n to
> i18n_ctype and then run `make all'.  The script tried not to change
> the comments and other lines which were already there.  I used
> a different technique: created an empty file i18n_ctype and run
> `make all' to make it fully generated from scratch.

Correct.

> Also see more here:
> 
>> +LC_IDENTIFICATION
>> +title                 ""
>> +source                ""
>> +address               ""
>> +contact               ""
>> +email                 ""
>> +tel                   ""
>> +fax                   ""
>> +language              ""
>> +territory             ""
>> +revision              ""
>> +date                  "2017-10-11"
>> +
>> +category "i18n:2012";LC_IDENTIFICATION
>> +category "i18n:2012";LC_CTYPE
>> +category "i18n:2012";LC_COLLATE
>> +category "i18n:2012";LC_TIME
>> +category "i18n:2012";LC_NUMERIC
>> +category "i18n:2012";LC_MONETARY
>> +category "i18n:2012";LC_MESSAGES
>> +category "i18n:2012";LC_PAPER
>> +category "i18n:2012";LC_NAME
>> +category "i18n:2012";LC_ADDRESS
>> +category "i18n:2012";LC_TELEPHONE
>> +END LC_IDENTIFICATION
> 
> My result here is:
> 
> LC_IDENTIFICATION
> title     "Unicode 10.0.0 FDCC-set"
> source    "UnicodeData.txt, DerivedCoreProperties.txt"
> address   ""
> contact   ""
> email     "bug-glibc-locales@gnu.org"
> tel       ""
> fax       ""
> language  ""
> territory "Earth"
> revision  "10.0.0"
> date      "2017-10-22"
> category  "unicode:2014";LC_CTYPE
> END LC_IDENTIFICATION
> 
> I'm not sure if these differences are really relevant so I don't
> mind if you refuse accepting my suggestions.

Not at all. Your changes look good!

Please feel free to adjust the file accordingly.

Something like this?

diff --git a/localedata/locales/i18n_ctype b/localedata/locales/i18n_ctype
index da88efc..a4ecc65 100644
--- a/localedata/locales/i18n_ctype
+++ b/localedata/locales/i18n_ctype
@@ -8,37 +8,26 @@ comment_char %
 % exempt you from the conditions of the license if your use would
 % otherwise be governed by that license.
 
-%
 % The only purpose for this file is to provide the basic Unicode
 % LC_CTYPE information.  This way other locales that need this
 % information, but with different transliterations, can include it
 % directly.
-%
 
-LC_IDENTIFICATION
-title                 ""
-source                ""
-address               ""
-contact               ""
-email                 ""
-tel                   ""
-fax                   ""
-language              ""
-territory             ""
-revision              ""
-date                  "2017-10-11"
+% Generated automatically by gen_unicode_ctype.py for Unicode 10.0.0.
 
-category "i18n:2012";LC_IDENTIFICATION
-category "i18n:2012";LC_CTYPE
-category "i18n:2012";LC_COLLATE
-category "i18n:2012";LC_TIME
-category "i18n:2012";LC_NUMERIC
-category "i18n:2012";LC_MONETARY
-category "i18n:2012";LC_MESSAGES
-category "i18n:2012";LC_PAPER
-category "i18n:2012";LC_NAME
-category "i18n:2012";LC_ADDRESS
-category "i18n:2012";LC_TELEPHONE
+LC_IDENTIFICATION
+title     "Unicode 10.0.0 FDCC-set"
+source    "UnicodeData.txt, DerivedCoreProperties.txt"
+address   ""
+contact   ""
+email     "bug-glibc-locales@gnu.org"
+tel       ""
+fax       ""
+language  ""
+territory "Earth"
+revision  "10.0.0"
+date      "2017-10-23"
+category  "unicode:2014";LC_CTYPE
 END LC_IDENTIFICATION
 
 LC_CTYPE
---

Looks good to me.

>> diff --git a/localedata/unicode-gen/Makefile b/localedata/unicode-gen/Makefile
>> index 4564670..32f4a53 100644
>> --- a/localedata/unicode-gen/Makefile
>> +++ b/localedata/unicode-gen/Makefile
>> @@ -20,7 +20,7 @@
>>  
>>  # This Makefile is NOT used as part of the GNU libc build.  It needs
>>  # to be run manually, within the source tree, at Unicode upgrades
>> -# (change UNICODE_VERSION below), to update ../locales/i18n ctype
>> +# (change UNICODE_VERSION below), to update ../locales/i18n_ctype ctype
>>  # information (part of the file is preserved, so don't wipe it all
>>  # out), and ../charmaps/UTF-8.
>>  
>> @@ -41,15 +41,15 @@ PYTHON3 = python3
>>  WGET = wget
>>  
>>  DOWNLOADS = UnicodeData.txt DerivedCoreProperties.txt EastAsianWidth.txt
>> PropList.txt
>> -GENERATED = i18n tr_TR UTF-8 translit_combining translit_compat
>> translit_circle translit_cjk_compat translit_font translit_fraction
>> -REPORTS = i18n-report UTF-8-report
>> +GENERATED = i18n_ctype tr_TR UTF-8 translit_combining translit_compat
>> translit_circle translit_cjk_compat translit_font translit_fraction
>> +REPORTS = i18n_ctype-report UTF-8-report
> 
> My comments about i18n_ctype-report file apply here as well.

This was on purpose. The report relates to the i18n_ctype file data only.

>>  
>>  all: $(GENERATED)
>>  
>> -check: check-i18n check-UTF-8
>> +check: check-i18n_ctype check-UTF-8
> 
> Here is the major problem: either you should consequently change
> check-i18n to check-i18n_ctype everywhere or not change it at all.
> Look below:

Thank you for spotting this!

>>  check-i18n: i18n-report
>>  	@if grep '\(Missing\|Added\) [^0]\|^Number of errors[^=]* = [^0]' \
>> -- 
>> 2.7.5
> 
> here check-i18n is not changed.  As a result, `make check' fails:
> 
> $ make check
> make: *** No rule to make target 'check-i18n_ctype', needed by 'check'.  Stop.
> 
> I will appreciate if you fix these problem on your own, the last problem
> at least.

I will fix this immediately and commit the changes. Thank you again for the review.

-- 
Cheers,
Carlos.