Bug 12061

Summary: tmpl_process3.pl new features
Product: Koha Reporter: Bernardo Gonzalez Kriegel <bgkriegel>
Component: I18N/L10NAssignee: Bernardo Gonzalez Kriegel <bgkriegel>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: f.demians, fridolin.somers, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7939    
Attachments: Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups
Bug 12061 - tmpl_process3.pl - Multiple source dir
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups
Bug 12061 - tmpl_process3.pl - Multiple source dir
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups
Bug 12061 - tmpl_process3.pl - Multiple source dir
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups
Bug 12061 - tmpl_process3.pl - Multiple source dir
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Include/exclude file by name
Bug 12061 - tmpl_process3.pl - Include/exclude file by name

Description Bernardo Gonzalez Kriegel 2014-04-10 04:01:03 UTC
New features on tmpl_process3.pl are needed to fulfill some tasks

a) Cleaning of PO files
-----------------------
When doing an update of translation files, old strings are preserved as comments (^#~). As a normal part of update this are removed with a script, and backup files removed.
Translators that use to do offline translations remove those comments

I propose a patch to remove obsoleted strings and backup files.
(see Bug 8257)

b) Multiple source dirs
-----------------------
Currently only files below one source dir are scanned for strings.

At this moment we have 3 opac files (prog/ccsr/bootstrap), but they share a lot of common strings (this date on master):
bootstrap 2282 strings
prog      2274 strings
ccsr      109  strings

But there are only 382 different strings between bootstrap/prog, and only 8 between prog/ccsr

When we update translation files we need to do the same translation on each file, not a big problem for near complete translations, but a big task for new languages.

An hypothetical unified PO file for all opac themes would have 2476 strings,
and we need to translate only once each string. I know that prog is programmed to be phased out soon, but each bugfix needs to be ported to each current theme, so the need to translate each file.

I propose a patch to add the ability to scan multiple source dirs to build a translation file, to make for example and unified opac PO file. This has not been requested by nobody yet, but is in my mind every time I translate.

c) Include/Exclude scanned files by name
----------------------------------------
Currently we can exclude "dir" names, which was used for Bug 7934.
But certain requests needs the ability to include/exclude files, for example we have a request to separate translations for each MARC dialect.

It's a valid request. Normally a language will only use one MARC dialect, and only one will use NORMARC :)

To do that we need a way to select which files to scan, or not scan (and also the previous feature, multiple source dirs)

I propose a patch to add this feature, choosing which files to include/exclude given some strings to match with filenames
Comment 1 Bernardo Gonzalez Kriegel 2014-04-10 04:17:28 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2014-04-10 04:17:31 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2014-04-10 04:17:35 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-05-22 14:14:01 UTC
Applying: Bug 12061 - tmpl_process3.pl - Multiple source dir
fatal: sha1 information is lacking or useless (misc/translator/tmpl_process3.pl).
Comment 5 Bernardo Gonzalez Kriegel 2014-05-22 14:28:27 UTC Comment hidden (obsolete)
Comment 6 Bernardo Gonzalez Kriegel 2014-05-22 14:28:36 UTC Comment hidden (obsolete)
Comment 7 Bernardo Gonzalez Kriegel 2014-05-22 14:28:45 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2014-05-22 14:30:06 UTC
Strange, for me they apply. 
Rebased anyway.
Comment 9 Jonathan Druart 2014-05-22 14:45:34 UTC
Comment on attachment 28445 [details] [review]
Bug 12061 - tmpl_process3.pl - Include/exclude file by name

Review of attachment 28445 [details] [review]:
-----------------------------------------------------------------

::: misc/translator/tmpl_process3.pl
@@ +200,5 @@
>    -s, --str-file=FILE         Specify FILE as the translation (po) file
>                                for input (install) or output (create, update)
> +  -x, --exclude=REGEXP        Exclude dirs matching the given REGEXP
> +  -m, --match=STRING          Pipe (|) separated list. Include only files
> +                              with names matching the given REGEXP

It looks to be the same behavior as the -f option.
Comment 10 Bernardo Gonzalez Kriegel 2014-05-23 01:19:32 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2014-05-23 01:25:48 UTC
(In reply to Jonathan Druart from comment #9)
> 
> It looks to be the same behavior as the -f option.

Yes... almost the same. Uploaded a revised version.
I chsnged '-f' to only look on filenames, currently it can match a dir name
(try -f help), also did the check case insensitive 

Removed -m, same -f function!

-n stay, same input as -f

changed usage description
Comment 12 Bernardo Gonzalez Kriegel 2014-05-23 01:27:45 UTC Comment hidden (obsolete)
Comment 13 Fridolin Somers 2014-05-27 09:35:37 UTC
Nice job.

I'm testing.
I think there is an error in test plan of "tmpl_process3.pl - Multiple source dir ":

3. install
mkdir test
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap -s bootstrap.po -r -o ./test -q
- check same number of files
ls -lR ../../koha-tmpl/opac-tmpl/bootstrap | wc -l
ls -lR ./test | wc -l

It should translate only "en" folder no ? otherwise there is not the same number of files : 

3. install
mkdir test
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s bootstrap.po -r -o ./test -q
- check same number of files
ls -lR ../../koha-tmpl/opac-tmpl/bootstrap/en | wc -l
ls -lR ./test | wc -l

Best regards.
Comment 14 Bernardo Gonzalez Kriegel 2014-05-27 12:39:47 UTC
(In reply to Fridolin SOMERS from comment #13)
> Nice job.
> 
> I'm testing.
> I think there is an error in test plan of "tmpl_process3.pl - Multiple
> source dir ":
> 
...
> 
> It should translate only "en" folder no ? otherwise there is not the same
> number of files : 
> 

Hi Fridolin, thanks for testing :)

No, tmpl_process3.pl do not look for 'en' dir. On create it looks (recursively -r) for valid files to pick strings from, on install it reproduces (translated or not, but all) files and dirs from input to target dir.
That test is only to check for regressions (all files present), because I touch the handling of source dir.

Just tested (with patches):
~/kohaclone/misc/translator$ ./tmpl_process3.pl create -i ../../koha-tmpl/opac-tmpl/bootstrap -s test.po -r -q
Warning: Can't determine original templates' charset, defaulting to UTF-8
Warning: Charset Out defaulting to UTF-8
Removing empty file test.po
I UTF-8 O UTF-8 at ./tmpl_process3.pl line 339.

~/kohaclone/misc/translator$ mkdir test

~/kohaclone/misc/translator$ ./tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap -s test.po -r -q -o ./test

~/kohaclone/misc/translator$ ls -lR ../../koha-tmpl/opac-tmpl/bootstrap/ | wc -l ; ls -lR ./test | wc -l
1951
1951
Comment 15 Fridolin Somers 2014-06-03 08:57:52 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #14)
> (In reply to Fridolin SOMERS from comment #13)
> > Nice job.
> > 
> > I'm testing.
> > I think there is an error in test plan of "tmpl_process3.pl - Multiple
> > source dir ":
> > 
> ...
> > 
> > It should translate only "en" folder no ? otherwise there is not the same
> > number of files : 
> > 
> 
> Hi Fridolin, thanks for testing :)
> 
> No, tmpl_process3.pl do not look for 'en' dir. On create it looks
> (recursively -r) for valid files to pick strings from, on install it
> reproduces (translated or not, but all) files and dirs from input to target
> dir.
> That test is only to check for regressions (all files present), because I
> touch the handling of source dir.
> 
> Just tested (with patches):
> ~/kohaclone/misc/translator$ ./tmpl_process3.pl create -i
> ../../koha-tmpl/opac-tmpl/bootstrap -s test.po -r -q
> Warning: Can't determine original templates' charset, defaulting to UTF-8
> Warning: Charset Out defaulting to UTF-8
> Removing empty file test.po
> I UTF-8 O UTF-8 at ./tmpl_process3.pl line 339.
> 
> ~/kohaclone/misc/translator$ mkdir test
> 
> ~/kohaclone/misc/translator$ ./tmpl_process3.pl install -i
> ../../koha-tmpl/opac-tmpl/bootstrap -s test.po -r -q -o ./test
> 
> ~/kohaclone/misc/translator$ ls -lR ../../koha-tmpl/opac-tmpl/bootstrap/ |
> wc -l ; ls -lR ./test | wc -l
> 1951
> 1951

My fault, I think I add not remove some previous tests.
Now it works.
Comment 16 Fridolin Somers 2014-06-03 09:02:52 UTC Comment hidden (obsolete)
Comment 17 Fridolin Somers 2014-06-03 09:03:24 UTC Comment hidden (obsolete)
Comment 18 Fridolin Somers 2014-06-03 09:03:37 UTC Comment hidden (obsolete)
Comment 19 Fridolin Somers 2014-06-03 09:04:16 UTC
All test plans work.
Perfect job.
Comment 20 Jonathan Druart 2014-06-06 14:51:26 UTC
Comment on attachment 28645 [details] [review]
Bug 12061 - tmpl_process3.pl - Include/exclude file by name

Review of attachment 28645 [details] [review]:
-----------------------------------------------------------------

::: misc/translator/tmpl_process3.pl
@@ +134,5 @@
>  
>  sub listfiles {
> +    my($dir, $type, $action) = @_;
> +    my $filenames   = join ('|', @filenames);
> +    my $nofilenames = join ('|', @nofilenames);

Don't you think it is better to pass filenames and nofilenames in parameter rather than using global vars?
Comment 21 Jonathan Druart 2014-06-06 14:53:28 UTC
Comment on attachment 28645 [details] [review]
Bug 12061 - tmpl_process3.pl - Include/exclude file by name

Review of attachment 28645 [details] [review]:
-----------------------------------------------------------------

::: misc/translator/tmpl_process3.pl
@@ +146,4 @@
>              || (defined $exclude_regex && $dirent =~ /^(?:$exclude_regex)$/)) {
>              ;
>              } elsif (-f $path) {
> +                my $basename = fileparse( $path );

hum, not sure to understand the change here.
Comment 22 Jonathan Druart 2014-06-06 14:54:57 UTC
Comment on attachment 28643 [details] [review]
Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups

Review of attachment 28643 [details] [review]:
-----------------------------------------------------------------

::: misc/translator/tmpl_process3.pl
@@ +377,4 @@
>          if ( @filenames ) {
>              my ($tmph3, $tmpfile3) = tmpnam();
>              $st = system("msgcat $str_file $tmpfile2 > $tmpfile3");
> +            $st = system("msgmerge ".($quiet?'-q':'')." -s $str_file $tmpfile3 -o - | msgattrib --no-obsolete -o $str_file")

Could you detail why this change is useful please? Maybe a small comment would be helpful.
Comment 23 Bernardo Gonzalez Kriegel 2014-06-07 11:56:24 UTC
(In reply to Jonathan Druart from comment #20)
> Comment on attachment 28645 [details] [review] [review]
> Bug 12061 - tmpl_process3.pl - Include/exclude file by name
> 
> Review of attachment 28645 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/translator/tmpl_process3.pl
> @@ +134,5 @@
> >  
> >  sub listfiles {
> > +    my($dir, $type, $action) = @_;
> > +    my $filenames   = join ('|', @filenames);
> > +    my $nofilenames = join ('|', @nofilenames);
> 
> Don't you think it is better to pass filenames and nofilenames in parameter
> rather than using global vars?

May be. What we can pass is a reference, so we are using them (glob vars) anyway.
My thinking was that we pass \@filenames, later use @$filenames, why not directly use @filenames?
If you think it's more clear to pass them explicitly, no problem from me, i'll rewrite that.
Comment 24 Bernardo Gonzalez Kriegel 2014-06-07 12:17:23 UTC
(In reply to Jonathan Druart from comment #21)
> Comment on attachment 28645 [details] [review] [review]
> Bug 12061 - tmpl_process3.pl - Include/exclude file by name
> 
> Review of attachment 28645 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/translator/tmpl_process3.pl
> @@ +146,4 @@
> >              || (defined $exclude_regex && $dirent =~ /^(?:$exclude_regex)$/)) {
> >              ;
> >              } elsif (-f $path) {
> > +                my $basename = fileparse( $path );
> 
> hum, not sure to understand the change here.

Currently $basename is declared and initialized, but not used.
My idea was to use this variable to match filenames, instead of using $path.
Current code could match a dir name if used with a generic term, e.g. "-f help" will match all help 'dirs' on staff, and is supposed to match filenames. 
But, if I understand correctly [1], it's better to use fileparse() for that instead of basename(). 


[1] http://perldoc.perl.org/File/Basename.html
Comment 25 Bernardo Gonzalez Kriegel 2014-06-07 12:36:08 UTC
(In reply to Jonathan Druart from comment #22)
> Comment on attachment 28643 [details] [review] [review]
> Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups
> 
> Review of attachment 28643 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/translator/tmpl_process3.pl
> @@ +377,4 @@
> >          if ( @filenames ) {
> >              my ($tmph3, $tmpfile3) = tmpnam();
> >              $st = system("msgcat $str_file $tmpfile2 > $tmpfile3");
> > +            $st = system("msgmerge ".($quiet?'-q':'')." -s $str_file $tmpfile3 -o - | msgattrib --no-obsolete -o $str_file")
> 
> Could you detail why this change is useful please? Maybe a small comment
> would be helpful.

There is a comment in bug description, point a), But no problem :) 

With every 'translate update ...' a msgid that is no more present is marked as obsolete '#~' and moved to the bottom. They have value if on time the same string is back again, then old translation could be used.
As translator I normally remove by hand those entries, and as TM I do the same for all files as part for the 'normal' procedure I inherited ([1], 'From Koha to Pootle').
On Bug 

[1] http://wiki.koha-community.org/wiki/Translation_Server
Comment 26 Bernardo Gonzalez Kriegel 2014-06-07 12:41:24 UTC
> On Bug ...
Sorry, was sent before finished.

There is also a comment about this on Bug 8257 (one of the open i18n bugs)
The idea then is save a little work and trim the files a bit removing obsolete strings.

Thanks for your comments :)
Comment 27 Jonathan Druart 2014-06-09 14:39:00 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #25)
> > Could you detail why this change is useful please? Maybe a small comment
> > would be helpful.
> 
> There is a comment in bug description, point a), But no problem :) 

I was thinking a comment in the code. But it could be retrieve from this bug report :)

(In reply to Bernardo Gonzalez Kriegel from comment #26)
> Thanks for your comments :)

Thanks for your answers, all look good and nothing is blocker in my opinion.

Marked as Passed QA.
Comment 28 Jonathan Druart 2014-06-09 14:40:48 UTC
Created attachment 28735 [details] [review]
Bug 12061 - tmpl_process3.pl - Remove obsoleted strings/backups

This patch modifies the update process for PO files,
removing obsoleted strings. Currently they are removed
manually as part of the standar procedure of strings update.
Also backup files (*~) need to be removed manually.

To test:
1. Update translation files for any language, ej. fi-FI
(cd misc/translation; perl translate update fi-FI)

2. Check existence of obsoleted strings
egrep "#~ msg" misc/translator/po/fi-FI-*

3. Check existence of backup files
ls misc/translator/po/fi-FI-*~

4. Reset to HEAD and clean

5. Apply the patch

6. Update again, check there are no more obsoleted
strings nor backup files

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 29 Jonathan Druart 2014-06-09 14:40:53 UTC
Created attachment 28736 [details] [review]
Bug 12061 - tmpl_process3.pl - Multiple source dir

This patch adds the option to use multiple source
dirs for tmpl_process3.pl on create/update actions.

To test after patch:
A) Old
1. install
cd misc/translator
perl tmpl_process3.pl create -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s bootstrap.po -r

2. update
perl tmpl_process3.pl update -i ../../koha-tmpl/opac-tmpl/bootstrap -s bootstrap.po -r

3. install
mkdir test
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap -s bootstrap.po -r -o ./test -q
- check same number of files
ls -lR ../../koha-tmpl/opac-tmpl/bootstrap | wc -l
ls -lR ./test | wc -l

rm -f bootstrap.po

B) New
4. create from multiple sources
perl tmpl_process3.pl create -i ../../koha-tmpl/opac-tmpl/prog/en -i ../../koha-tmpl/opac-tmpl/bootstrap/en -i ../../koha-tmpl/opac-tmpl/ccsr/en -s opac.po -r

5. update from multiple source
perl tmpl_process3.pl update -i ../../koha-tmpl/opac-tmpl/prog/en -i ../../koha-tmpl/opac-tmpl/bootstrap/en -i ../../koha-tmpl/opac-tmpl/ccsr/en -s opac.po -r

6. install (must give an error)
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/prog/en -i ../../koha-tmpl/opac-tmpl/bootstrap -s opac.po -r -o ./test

7. updated help
perl tmpl_process3.pl --help

Any other functionality must not be affected

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 30 Jonathan Druart 2014-06-09 14:41:00 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2014-06-09 14:44:26 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #24)
> But, if I understand correctly [1], it's better to use fileparse() for that
> instead of basename(). 

I always used basename! Thank for pointing that out to me.
Comment 32 Bernardo Gonzalez Kriegel 2014-06-20 14:00:32 UTC
Found a problem, do not push
Comment 33 Bernardo Gonzalez Kriegel 2014-07-26 16:38:39 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2014-07-26 17:06:48 UTC
(In reply to Jonathan Druart from comment #9)
...
> It looks to be the same behavior as the -f option.

After all, it is not the same behavior!

This code on 'update' is the main difference

if ( @filenames ) {
   my ($tmph3, $tmpfile3) = tmpnam();
   $st = system("msgcat $str_file $tmpfile2 > $tmpfile3");
   $st = system("msgmerge ".($quiet?'-q':'')." -s $str_file $tmpfile3 -o - | msgattrib --no-obsolete -o $str_file") unless $st;
} else {
  $st = system("msgmerge ".($quiet?'-q':'')." -s $str_file $tmpfile2 -o - | msgattrib --no-obsolete -o $str_file");
}
 
'-f' is the right choice to use when a file is modified and one updates translation file to reflect that.
But that is not the behavior that I want, PO file must reflect exactly the contents of source dir on create & update.

I retained Fridolin sign, he signed a previous version with both (-n and -m) new options, the code is slightly different but closer to the version that you QAed.

Left as signed for your consideration, but only changed the last patch.
Comment 35 Jonathan Druart 2014-08-19 08:33:00 UTC
Created attachment 30966 [details] [review]
Bug 12061 - tmpl_process3.pl - Include/exclude file by name

This patch adds options to include/exclude files by matching their names.
Also modifies current code to check only filenames (not dirs)
Checking is case insensitive !!

NOTE: The difference between -f and -m is subtle, but important.
They differs mainly on update,: -f do a merge, -m a replace

To test after patch:

A) Include only 'normarc'
1. create
cd misc/translator
perl tmpl_process3.pl create -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s normarc.po -r -m normarc
- check provenance of strings
egrep "^#:" normarc.po | cut -d":" -f2 | sort | uniq
- only files with normarc in their names must be present

2. update
perl tmpl_process3.pl update -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s normarc.po -r -m normarc
- repeat check

3. install
mkdir test
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap/en -o ./test -s normarc.po -r -m normarc
- check name of created files

rm -rf test normarc.po

B) Exclude
4. create
perl tmpl_process3.pl create -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s xnormarc.po -r -n normarc
- check provenance
egrep "^#:" xnormarc.po | cut -d":" -f2 | sort | uniq | grep -i normarc
- there must be no results

5. update
perl tmpl_process3.pl update -i ../../koha-tmpl/opac-tmpl/bootstrap/en -s xnormarc.po -r -n normarc
- check provenance

6. install
mkdir test
perl tmpl_process3.pl install -i ../../koha-tmpl/opac-tmpl/bootstrap/en -o ./test -s xnormarc.po -r -n normarc
- check files
find test | grep -i normarc
- there must be no results

You can also try another combination, use for example "-m patron -m user -m bottom" (or use -n)
or mixed "-m marc -n normarc", do create/install and look filenames

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 36 Tomás Cohen Arazi 2014-09-01 14:08:08 UTC
Patches pushed to master.

Thanks Bernardo!