304417 - Template precompilation is clumsy when it comes to additional language directories

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Marc Schumann [:Wurblzap]]

The precompilation code correctly rounds up all .tmpl files, and then compiles
each like a browser giving no Accept-Language header would.

-> Only "en" templates end up being precompiled in data/template/template
-> Other language directories, custom directories and extension directories
   each cause precompilations with no net effect:
   en/custom/index.html.tmpl is compiled for en/default/index.html.tmpl,
   xx/default/index.html.tmpl, xx/custom/index.html.tmpl and of course
   en/custom/index.html.tmpl
-> Localized template packs may not introduce additional language-specific
   templates:
   if they try to, precompilation errors out saying "file error -
   somewhere/whatever.html.tmpl: not found" because the template is being
   looked for in template/en only.

Suggested solution:
Template precompilation should precompile each language directory separately.
For each language directory in turn, it should
 - make a list of all .tmpl files in there
 - remove duplicates between default, custom and extension from the list
 - set $ENV{'HTTP_ACCEPT_LANGUAGE'} to the name of the language directory
 - perform precompilation for each .tmpl file left on the list

Comment 1

[Jochen Wiedmann]

Attachment: Suggested patch (see comment for details)

The patch does the following:

- Adds a parameter clean_cache Bugzilla::Template->create. This
  parameter clears the cache of template directories.
- Splits the template generation loop into an outer and an
  inner loop. The outer loop is per language, the inner loop
  is per template directory. (default, custom, extensions)
- For any language, a new instance of Bugzilla::Template is
  instantiated. The environment variable HTTP_ACCEPT_LANGUAGE
  and the parameter 'languages' are configured properly.

Comment 2

[Jochen Wiedmann]

Suggested blocking 2.20, because the bug prevents installation of germzilla
(german localization, see http://germzilla.wurblzap.net).

Comment 3

[Max Kanat-Alexander]

This is a fairly significant checksetup change, so it doesn't block the release.
We *might* take it for the branch (2.20.1), but I'd have to see what the
reviewers think.

Comment 4

[Marc Schumann [:Wurblzap]]

I agree it's not a blocker.

This particular part of the checksetup.pl code has been pretty static for quite
a while, though, so it seems to me to be good for the branch, perhaps even both
branches. I'd say let's depend it on how difficult a backport turns out to seem,
and let's optimistically set the milestone to 2.18.

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 194998 [details] [diff] [review]
Suggested patch (see comment for details)

Good patch. It's mostly all right...

>+++ ./Bugzilla/Template.pm	2005-09-06 12:28:51.000000000 +0200
>@@ -197,6 +197,10 @@
> 
> sub create {
>     my $class = shift;
>+    my %opts = @_;
>+    if ($opts{clean_cache}) {
>+        $template_include_path = undef;
>+    }

Please put a comment here... It took me a while to figure out what's going on.

In fact the $template_include_path global, being a global, is a Bad Thing and
shouldn't exist at all, and the "create" sub shouldn't be aware of its
existence, but all that is not your fault :)

It may be just me, but I think the clean_cache string should be quoted as
'clean_cache'. There are some other barewords in your patch, too, which should
be quoted as well while we're here. For reference and why I think so, see the
Barewords section in http://perldoc.perl.org/perldata.html.

>+++ ./checksetup.pl	2005-09-06 12:30:17.000000000 +0200

>+            SetParam("languages", "$dir,en");

I'd think precompilation should work with the original param untouched... Are
you using this line for a reason?
Maybe instead of saying |@files = grep readdir| -- might it be better to have
|@files = split(/\s*,\s*/, Param('languages')| so that the language pack you're
trying to precompile is part of the search path?

>+            my $path = File::Spec->catdir($templatedir, $dir, 'custom');
>+            push(@templatepaths, $path) if(-d $path);
>+            $path = File::Spec->catdir($templatedir, $dir, 'extension');
>+            push(@templatepaths, $path) if(-d $path);
>+            $path = File::Spec->catdir($templatedir, $dir, 'default');
>+            push(@templatepaths, $path) if(-d $path);

Nit: if you want, convert this code into a |foreach my $subdir ('custom',
'extension', 'default')| loop. It's not your code, I know...

>         foreach $::templatepath (@templatepaths) {
>            # Traverse the template hierarchy. 
>            find({ wanted => \&compile, no_chdir => 1 }, $::templatepath);
>        }
>     }
>+    }

Please correct indentation here.

You're copying code instead of moving it. You should remove the original code
because it's not needed any more. This kills a code comment, too, which you
should imho consider updating and moving to your code to explain what you're
doing.

Comment 6

[Marc Schumann [:Wurblzap]]

Jochen, there is not much missing to your patch to go in... Do you think you can come up with new one in time for 2.22?

Comment 7

[Jochen Wiedmann]

Attachment: Updated patch, considering Marc's comments

Comment 8

[Jochen Wiedmann]

Note, that I wasn't able to run the patch against the current HEAD, which is currently unusable on my machine. It works on 2.20.

Comment 9

[Marc Schumann [:Wurblzap]]

Next time, please provide a unified diff (parameter -u). Unified diffs don't rot as fast, and are more likely to apply to both trunk and branch. Your first diff was just fine :)

> >+            SetParam("languages", "$dir,en");
> 
> I'd think precompilation should work with the original param untouched... Are
> you using this line for a reason?
> Maybe instead of saying |@files = grep readdir| -- might it be better to have
> |@files = split(/\s*,\s*/, Param('languages')| so that the language pack
> you're trying to precompile is part of the search path?

Can you please give me an answer on this? I'm still not clear why we should do it the SetParam way.

Comment 10

[Jochen Wiedmann]

Without the

    SetParam("languages", "$dir,en");

I still receive the error message

    file error - global/gzversion.html.tmpl: not found

Comment 11

[Marc Schumann [:Wurblzap]]

With regard to your @developers posting -- your patch here is indeed very useful and very helpful. In fact, it's nearly there. If you want to skip polishing and formalism, then I'm ready and willing to take over and pull the patch through from here. On the other hand, if you wish to see it through, then I'm positive that we'll get it reviewed (and possibly checked in) by the end of next week :)

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 201664 [details] [diff] [review]
Updated patch, considering Marc's comments

>>         use File::Spec; 
>>         opendir(DIR, $templatedir) || die "Can't open '$templatedir': $!";
>>         my @files = grep { /^[a-z-]+$/i } readdir(DIR);
>>         closedir DIR;

Hmmm... The way I see it, there are two reasons for template precompilation in checksetup.pl.
One is that it doesn't have to happen at runtime. The other is that the admin can see whether all templates compile correctly. Compilation errors don't show up in the web server's error log.

I think we should therefore take care that the setting in which precompilation happens is as close to reality as possible. This means we should precompile only language dirs which are configured to be allowed by the language param. Currently, we don't get errors if there is a language configured which is not installed. And it means we should leave the language param untouched for the compilation itself -- if an error occurs with an unmodified language param, we want to see it at checksetup.pl time, because it'd cause errors at runtime. Setting the parameter currently hides the error.

So... I think it'd be best not to look at $templatedir contents. Instead, we should fill @files with the contents of the languages param, using split(/\s*,\s*/, Param('languages').
We should be able to remove the SetParam call then.

If this is worked out, the patch is ready to go.

Comment 13

[Jochen Wiedmann]

Marc, please note, that you are clearly extending the original scope of this bug by changing the intended logic.

The current implementation compiles all available templates. So does the patched version.

Besides, I am uneasy about the intended work flow. Wouldn't that force me to enable a language even before the templates were compiled? And would that actually work? I remember a case where I had configured an invalid language. The result was that I had to reset the parameter manually, because Bugzilla was actually unusable.

Comment 14

[Marc Schumann [:Wurblzap]]

Comment on attachment 201664 [details] [diff] [review]
Updated patch, considering Marc's comments

You're right about the scope extension. It's good practice, though, to fix other stuff encountered along the way.

I agree that it is generally a good idea to activate a language *after* a successful checksetup.pl run. This leaves us with the assumption that language packs are allowed to fall back to English, and we won't catch all errors. I can't think of a good solution to this, though, so let's leave it for another bug.

The patch covers this bug, fixing the localized language pack issues. Moreover, it does improve precompilation a lot, so r=wurblzap.

Comment 15

[Marc Schumann [:Wurblzap]]

The affected code has not changed since 2.20, letting it go on the branch could therefore be done imo. I'm even all for a 2.18 backport. Dave?

Comment 16

[Dave Miller [:justdave]]

yeah, I'll go for a 2.18 backport, as long as we've tested the hell out of it and it doesn't break anything.  It certainly sounds like it fixes a lot.

Comment 17

[Marc Schumann [:Wurblzap]]

That's a relief :)
Jochen, do you want to do the backport?

Comment 18

[Jochen Wiedmann]

My time currently goes into preparing the alpha release of Apache XML-RPC 3. If that is done (some weeks) I am ready to do a 2.18 installation.

Comment 19

[Jochen Wiedmann]

Marc, I installed bugzilla-2.18.4 and germzilla-2.18.4-1.utf-8 today. I could not reproduce the problem: After setting the parameters properly I am greeted by a german Bugzilla. (Note, that I did *not* set HTTP_ACCEPT_LANGUAGE, or something like that.)

Comment 20

[Marc Schumann [:Wurblzap]]

Ok... It turns out this is because bug 288527 didn't make it into 2.18.

Comment 21

[Marc Schumann [:Wurblzap]]

Tip:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.452; previous revision: 1.451
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.38; previous revision: 1.37
done

Branch:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412.2.11; previous revision: 1.412.2.10
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.26.2.2; previous revision: 1.26.2.1
done

Comment 22

[Teemu Mannermaa (:wicked)]

This patch was incorrectly applied to the 2.20 branch (works on tip). Now every page log an error like "index.cgi: Odd number of elements in hash assignment at Bugzilla/Template.pm line 287." This is because this patch applied a hunk at line 287 which is inside no_break filter on the 2.20 branch! Please move the hunk at correct location.

This is why I prefer unified diffs which CVS can atleast try sync to correct line..

Comment 23

[Teemu Mannermaa (:wicked)]

Attachment: Fix Bugzilla/Template.pm commit, 2.20 branch

Here's a patch that fixes the bad commit of Bugzilla/Template.pm change done to 2.20 branch.

Comment 24

[Teemu Mannermaa (:wicked)]

Attachment: Fix Bugzilla/Template.pm commit, tip

I double checked and tip commit was incorrect too. Here's a patch that fixes the bad commit of Bugzilla/Template.pm change done to tip.

Comment 25

[Frédéric Buclin]

Fixing incorrect checkin.

tip:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.39; previous revision: 1.38
done

2.20:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.26.2.3; previous revision: 1.26.2.2
done