383522 - mod_perl creates a separate template directory

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P3)

Description

[Greg Hendricks]

Running under mod_perl, a second copy of the compiled templates are being placed in  data/templates under an absolute path to the template files. 
for example, if my bugzilla installation is located at /srv/bugzilla/htdocs I see 

data/
|_templates/
  |_en/
  | |_default/
  |
  templates/
  |_srv/
    |_bugzilla/ 
      |_htdocs/
        |_template/
          |_en/
            |_default/
etc.

These are owned by the web server so when I run checksetup it complains that it can't delete the templates and I have to do so manually.

Comment 1

[Max Kanat-Alexander]

This could actually be indicating some deeper problem in Bugzilla::Template, which we should at least investigate before 3.0.1.

Comment 2

[Max Kanat-Alexander]

Okay, so this is not hiding any deeper problem in our code, it's just that mod_perl won't use the pre-compiled templates.

You can read about the Template-Toolkit COMPILE_DIR option here, which will explain things:

http://search.cpan.org/dist/Template-Toolkit/lib/Template/Manual/Config.pod#Caching_and_Compiling_Options

One solution would be to make bz_locations always use absolute paths. We could only do that by calling abs_path, which hits the filesystem and could also die if people can't chdir to certain directories above theirs. (Although I'm not sure why that would ever happen.)

Comment 3

[Max Kanat-Alexander]

I've realized that the real solution here is for checksetup to symlink data/template/var/www/html/mod_perl/data/template to data/template/template. That is, do a symlink from the absolute path to the normal path, if we're on a system that allows symlinks. If we're on Windows, (a) we don't support mod_perl and (b) they'll just have to re-compile their templates.

Comment 4

[Max Kanat-Alexander]

Attachment: v1

This fixes it and works on landfill.

Comment 5

[Max Kanat-Alexander]

Attachment: v2 - 3,0

Oh, oops. We were symlinking to data/template instead of data/template/template.

Comment 6

[Greg Hendricks]

Comment on attachment 283153 [details] [diff] [review]
v2 - 3,0

This looks like it should work, but I haven't had a chance to test it yet. My test box had to be rebuilt so I need to set up the mod_perl environment again before I can.

Comment 7

[Frédéric Buclin]

Comment on attachment 283153 [details] [diff] [review]
v2 - 3,0

Unless I miss something, you create the symlink even if you are not running mod_perl. Also, you try to create it all the time. Is this not going to fail if the symlink already exists?

Comment 8

[Greg Hendricks]

(In reply to comment #7)
> (From update of attachment 283153 [details] [diff] [review])
> Unless I miss something, you create the symlink even if you are not running
> mod_perl. Also, you try to create it all the time. Is this not going to fail if
> the symlink already exists?
> 

It would be harmless even if you are not running mod_perl, though I suppose a test for that would be simple enough. The eval block will catch if the symlink already exists and warn, but how hard would it be to check that it doesn't already exist?

Comment 9

[Max Kanat-Alexander]

(In reply to comment #7)
> (From update of attachment 283153 [details] [diff] [review])
> Unless I miss something, you create the symlink even if you are not running
> mod_perl. 

  You did miss something: We're running in checksetup. From the command line.

  As Greg said, the symlink is totally harmless for non-mod_perl installations.

> Also, you try to create it all the time. Is this not going to fail if
> the symlink already exists?

  It will never already exist--the "rmtree" above takes care of it, by deleting the whole data/template directory.

Comment 10

[Frédéric Buclin]

Comment on attachment 283153 [details] [diff] [review]
v2 - 3,0

>+    mkpath("$datadir/template$abs_root");
>+    my $todir   = "$datadir/template$abs_root";

You should shift these two lines so that you can write mkpath($todir).


>+    eval { symlink($fromdir, "$todir/template") 
>+               or warn "Failed to symlink from $fromdir to $todir: $!" };

Nit: you should get the return value of symlink() and make sure this value is 1 (success). If symlink() is available but couldn't create the link, it returns 0.


One thing I would like an answer before you check in this patch: what happens if your web browser is configured to not follow symlinks, i.e.  |Options FollowSymLinks| is not set? Also, are we sure you can always follow symlinks which have ../../ in them (security)?

r=LpSolit assuming you tested it with mod_perl enabled, but I would like a 2nd review as I'm unsure about my questions above.

Comment 11

[Dave Miller [:justdave]]

I wouldn't think FollowSymlinks would matter as it's Buzilla code (Template Toolkit specifically) following the symlink and not Apache.

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #2)
> Okay, so this is not hiding any deeper problem in our code, it's just that
> mod_perl won't use the pre-compiled templates.

So how would that other patch that fixes the namespace problem affect this?

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> So how would that other patch that fixes the namespace problem affect this?

  It won't make any difference.

(In reply to comment #10)
> You should shift these two lines so that you can write mkpath($todir).

  Good idea, I can do that on checkin.

> >+    eval { symlink($fromdir, "$todir/template") 
> >+               or warn "Failed to symlink from $fromdir to $todir: $!" };
> 
> Nit: you should get the return value of symlink() and make sure this value is 1

  I do. That's what "or warn" is there for.

> One thing I would like an answer before you check in this patch: what happens
> if your web browser is configured to not follow symlinks, i.e.  |Options
> FollowSymLinks| is not set?

  As justdave said, the web browser isn't following any symlinks, just Perl is, so it's fine.

> Also, are we sure you can always follow symlinks which have ../../ in them 
> (security)?

  I'm not aware of any security restriction which would prevent it.

> r=LpSolit assuming you tested it with mod_perl enabled, but I would like a 2nd
> review as I'm unsure about my questions above.

  It's running on bugzilla.everythingsolved.com right now.

Comment 14

[Frédéric Buclin]

Comment on attachment 283153 [details] [diff] [review]
v2 - 3,0

ok, go ahead.

Comment 15

[Max Kanat-Alexander]

tip:

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

3.0:

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

Comment 16

[Max Kanat-Alexander]

Attachment: v2 - tip

There was actually some bitrot on the tip--this is what I checked in on the tip.

Comment 17

[Frédéric Buclin]

Running checksetup.pl on Windows throws the following error:

Removing existing compiled templates ...
Precompiling templates...
Argument "Invalid argument; Syntaxe du nom de fichier, de répertoire..." isn't numeric in scalar assignment at C:/Perl/lib/File/Path.pm line 152.
mkdir ./data/templateC:/: Invalid argument; Syntaxe du nom de fichier, de répertoire ou de volume incorrecte at Bugzilla/Template.pm line 795

Bugzilla/Template.pm line 795 is:

 mkpath("$datadir/template$abs_root");

File::Path complains because ":" is invalid in a directory name, and so you cannot write data/templateC:/. Shouldn't we skip this whole block if we are running Windows?

Comment 18

[Frédéric Buclin]

Marking this bug as blocking 3.0.3 as checksetup.pl fails on Windows. Workaround: call checksetup.pl with -t, but we cannot leave this bug in a stable release.

Comment 19

[Frédéric Buclin]

(In reply to comment #13)
> > You should shift these two lines so that you can write mkpath($todir).
> 
>   Good idea, I can do that on checkin.

This hasn't been fixed on checkin. Please include it when fixing this bug for Windows.

Comment 20

[Max Kanat-Alexander]

Attachment: Fix For Windows

This should fix the Windows problem.

Comment 21

[Frédéric Buclin]

Comment on attachment 286884 [details] [diff] [review]
Fix For Windows

r=LpSolit

Comment 22

[Max Kanat-Alexander]

tip:

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

3.0 branch:

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