mod_perl creates a separate template directory

RESOLVED FIXED in Bugzilla 3.0

Status

()

P3
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: gregaryh, Assigned: mkanat)

Tracking

({perf})

Bugzilla 3.0
Bug Flags:
approval +
blocking3.1.3 +
approval3.0 +
blocking3.0.3 +
blocking3.0.1 -

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
This could actually be indicating some deeper problem in Bugzilla::Template, which we should at least investigate before 3.0.1.
Flags: blocking3.0.1+
Priority: -- → P1
(Assignee)

Updated

11 years ago
Assignee: administration → mkanat

Updated

11 years ago
Target Milestone: --- → Bugzilla 3.0
(Assignee)

Comment 2

11 years ago
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.)
Flags: blocking3.0.1+ → blocking3.0.1-
(Assignee)

Updated

11 years ago
OS: Linux → All
Hardware: PC → All
(Assignee)

Updated

11 years ago
Priority: P1 → P3
(Assignee)

Comment 3

11 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 years ago
Created attachment 283147 [details] [diff] [review]
v1

This fixes it and works on landfill.
Attachment #283147 - Flags: review?(ghendricks)
Attachment #283147 - Flags: review?(LpSolit)
(Assignee)

Updated

11 years ago
Keywords: perf
(Assignee)

Comment 5

11 years ago
Created attachment 283153 [details] [diff] [review]
v2 - 3,0

Oh, oops. We were symlinking to data/template instead of data/template/template.
Attachment #283147 - Attachment is obsolete: true
Attachment #283153 - Flags: review?(ghendricks)
Attachment #283153 - Flags: review?(LpSolit)
Attachment #283147 - Flags: review?(ghendricks)
Attachment #283147 - Flags: review?(LpSolit)
(Reporter)

Comment 6

11 years ago
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

11 years ago
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?
(Reporter)

Comment 8

11 years ago
(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?
(Assignee)

Comment 9

11 years ago
(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

11 years ago
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.
Attachment #283153 - Flags: review?(justdave)
Attachment #283153 - Flags: review?(LpSolit)
Attachment #283153 - Flags: review+
I wouldn't think FollowSymlinks would matter as it's Buzilla code (Template Toolkit specifically) following the symlink and not Apache.
(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?
(Assignee)

Comment 13

11 years ago

(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

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

ok, go ahead.
Attachment #283153 - Flags: review?(justdave)
Attachment #283153 - Flags: review?(ghendricks)
(Assignee)

Updated

11 years ago
Flags: approval3.0+
Flags: approval+
(Assignee)

Comment 15

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
Created attachment 285075 [details] [diff] [review]
v2 - tip

There was actually some bitrot on the tip--this is what I checked in on the tip.
Attachment #285075 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #283153 - Attachment description: v2 → v2 - 3,0

Comment 17

11 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 18

11 years ago
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.
Flags: blocking3.1.3+
Flags: blocking3.0.3+

Comment 19

11 years ago
(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.
(Assignee)

Comment 20

11 years ago
Created attachment 286884 [details] [diff] [review]
Fix For Windows

This should fix the Windows problem.
Attachment #286884 - Flags: review?(LpSolit)
(Assignee)

Updated

11 years ago
Severity: normal → major

Comment 21

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

r=LpSolit
Attachment #286884 - Flags: review?(LpSolit) → review+
(Assignee)

Comment 22

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.